MM-69893: Mount the host WYSIWYG editor at the page route with draft autosave - #13
Merged
Conversation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
webapp/src/components/page_editor/page_editor.module.scss (1)
1-10: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the blank line inside
.root.Stylelint reports
declaration-empty-line-beforeat Line 7. The blank Line 6 betweenmin-width: 0;andmin-height: 0;triggers this.🧹 Proposed fix
.root { display: flex; flex: 1 1 0; flex-direction: column; min-width: 0; - min-height: 0; height: 100%; background: var(--center-channel-bg); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webapp/src/components/page_editor/page_editor.module.scss` around lines 1 - 10, Remove the empty line between the min-width and min-height declarations in the .root style rule, leaving the declaration order and values unchanged.Source: Linters/SAST tools
🧹 Nitpick comments (8)
webapp/src/client/presence_events.ts (1)
21-23: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueIsolate listener errors during publish.
publishPagePresencecalls each listener directly. If one listener throws, the remaining listeners never receive the event, and the exception propagates into the WebSocket handler registered inwebapp/src/index.tsx. Wrap each call so one failing subscriber cannot block delivery.♻️ Proposed isolation of listener errors
export function publishPagePresence(event: PagePresenceEvent): void { - listeners.forEach((listener) => listener(event)); + listeners.forEach((listener) => { + try { + listener(event); + } catch { + // A single subscriber must not block delivery to the others. + } + }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webapp/src/client/presence_events.ts` around lines 21 - 23, Update publishPagePresence so each listener invocation is isolated with per-listener error handling, ensuring an exception from one subscriber does not stop iteration or propagate into the WebSocket handler; preserve delivery of the event to all remaining listeners.webapp/src/hooks/draft_autosave.ts (1)
112-124: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winThe debounce has no maximum wait, so continuous typing never saves.
Every
queuecall clears the timer and starts a new one. While a user types without a pause ofAUTOSAVE_DEBOUNCE_MS, no write occurs. A long uninterrupted editing session therefore holds all content in memory. A tab crash or a forced reload loses that work.Consider tracking the time of the first pending edit and forcing a write once a maximum interval elapses.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webapp/src/hooks/draft_autosave.ts` around lines 112 - 124, The queue function’s debounce can be postponed indefinitely during continuous edits. Add tracking for when the current pending batch first starts, and in queue enforce a maximum wait interval that triggers write even when the debounce timer keeps resetting; reset that tracking when the pending changes are written or cleared, while preserving the existing debounce behavior for shorter pauses.webapp/src/hooks/draft_autosave.test.tsx (1)
266-280: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd baseline coverage to the page-change test.
This test omits
baseEditAt, so it asserts only that the patch reachespage1. It cannot detect whichbase_edit_ataccompanies that patch. Two gaps remain untested, and both correspond to issues raised onwebapp/src/hooks/draft_autosave.ts:
- Rerender with a different
baseEditAttogether with the newpageId, then assert the flushed patch carries the previous page'sbase_edit_at.- Add a case with
baseEditAt: 0and assertbase_edit_at: 0is still sent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webapp/src/hooks/draft_autosave.test.tsx` around lines 266 - 280, Expand the page-change coverage around setup and the “flushes the pending patch…” test to provide an initial baseEditAt, rerender with both a new pageId and different baseEditAt, and assert the flushed page1 patch includes the previous page’s base_edit_at. Add a separate case using baseEditAt: 0 and verify the emitted patch preserves base_edit_at: 0.webapp/src/components/page_editor/toolbar_controls.tsx (2)
138-146: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove
CALLOUT_LABELSaboveCalloutControland store plain message descriptors.
CalloutControlreadsCALLOUT_LABELSat line 128, but the constant is declared here at line 140. The reference resolves at render time, so it works today. Declaring the constant before its use removes the forward reference. StoringMessageDescriptorvalues instead of functions also removes theFormatterindirection.♻️ Proposed refactor
-type Formatter = ReturnType<typeof useIntl>['formatMessage']; - -const CALLOUT_LABELS: Record<CalloutType, (f: Formatter) => string> = { - info: (f) => f({id: 'docs.editor.calloutInfo', defaultMessage: 'Info'}), - note: (f) => f({id: 'docs.editor.calloutNote', defaultMessage: 'Note'}), - success: (f) => f({id: 'docs.editor.calloutSuccess', defaultMessage: 'Success'}), - warning: (f) => f({id: 'docs.editor.calloutWarning', defaultMessage: 'Warning'}), - error: (f) => f({id: 'docs.editor.calloutError', defaultMessage: 'Error'}), -};Add above
CalloutControl:const CALLOUT_LABELS: Record<CalloutType, MessageDescriptor> = { info: {id: 'docs.editor.calloutInfo', defaultMessage: 'Info'}, note: {id: 'docs.editor.calloutNote', defaultMessage: 'Note'}, success: {id: 'docs.editor.calloutSuccess', defaultMessage: 'Success'}, warning: {id: 'docs.editor.calloutWarning', defaultMessage: 'Warning'}, error: {id: 'docs.editor.calloutError', defaultMessage: 'Error'}, };Then render with
{formatMessage(CALLOUT_LABELS[type])}.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webapp/src/components/page_editor/toolbar_controls.tsx` around lines 138 - 146, Move CALLOUT_LABELS above CalloutControl and change its values from formatter functions to MessageDescriptor objects. Remove the Formatter indirection, and update CalloutControl to pass CALLOUT_LABELS[type] directly to formatMessage when rendering the label.
112-133: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd arrow-key navigation to the callout menu.
The container declares
role='menu'and the items declarerole='menuitem'. A screen reader then announces a menu, and the user expects arrow keys to move between items. The current code provides no arrow-key handling and no rovingtabindex. Tab still reaches each button, so the task stays completable, but the announced interaction model does not match the behaviour.Either implement arrow-key navigation with a roving
tabindex, or drop therole='menu'androle='menuitem'attributes and let the buttons present as a plain group.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webapp/src/components/page_editor/toolbar_controls.tsx` around lines 112 - 133, Align the callout menu’s accessibility semantics with its behavior in the toolbar component: either implement arrow-key navigation and roving tabindex for the `menu` and `menuitem` elements around `CALLOUT_TYPES.map`, or remove those role attributes so the controls remain a plain button group. Preserve the existing `insert(type)` activation behavior.webapp/src/hooks/caret_anchored_suggestions.ts (1)
6-8: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe
.suggestion-listselector couples this hook to a host-owned class name.The hook positions a DOM node that the host web application renders. If the host renames the class, then this code silently stops positioning the list and no error appears. Add a comment that records the host version this selector targets, so a future reader can trace the dependency.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webapp/src/hooks/caret_anchored_suggestions.ts` around lines 6 - 8, Add a concise comment next to the SELECTOR constant documenting the host version associated with the `.suggestion-list` class, preserving the existing selector and positioning behavior.webapp/src/components/page_editor/publish_conflict_dialog.tsx (1)
17-23: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCompare the conflict reason exactly instead of by substring.
Line 23 uses
reason.includes('concurrent_autosave'). Substring matching also matches an unrelated future reason such asnot_concurrent_autosaveor a reason that embeds the token in a longer message. Typereasonas a union of the server reason codes and compare with===. The only consequence today is the wrong explanatory paragraph, so this is a robustness improvement rather than a defect.#!/bin/bash # Find the reason values that the server and the draft client produce. rg -n 'concurrent_autosave|PublishConflictError|reason' --type=ts --type=tsx -g '!**/*.test.*' rg -n 'concurrent_autosave' --type=go🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webapp/src/components/page_editor/publish_conflict_dialog.tsx` around lines 17 - 23, Update isConcurrentAutosave to accept the server’s reason-code union rather than a generic string, and compare the value exactly with === 'concurrent_autosave'. Reuse the existing reason-code type or define the union from the server/draft-client reason values, preserving the current explanatory-paragraph behavior for the exact concurrent_autosave code.webapp/src/components/page_editor/page_editor.module.scss (1)
28-32: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider consolidating the repeated
.columnselector.
.columnis declared three times (Lines 28-32, 62-73, 88-153). Merging these into a single block groups the editor-surface and callout styling with the layout rules, and makes future edits less likely to miss one of the declarations.Also applies to: 62-73, 88-153
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webapp/src/components/page_editor/page_editor.module.scss` around lines 28 - 32, Consolidate the three `.column` declarations into one selector block, combining the layout, editor-surface, and callout rules while preserving all existing properties and responsive behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@webapp/src/components/docs_root/docs_main_content.tsx`:
- Around line 24-44: Update DocsMainContent and the PageEditor loading flow to
explicitly validate that the requested space exists before rendering an editor
for spaceId/pageId. Preserve the existing 404 handling that allows valid new
drafts, but prevent invalid spaceId requests from rendering an empty editor when
both requests return 404; reuse the existing space state and error-handling
symbols.
In `@webapp/src/components/page_editor/apply_formatting.ts`:
- Around line 21-43: The selectWordUnderCaret function uses textContent, which
omits inline leaf nodes while parentOffset counts them. Replace the parent text
extraction with parent.textBetween(0, parent.content.size, undefined, '\ufffc')
so text indexing and caret offsets share one coordinate space; preserve the
existing word-boundary and selection logic.
In `@webapp/src/components/page_editor/callout_extension.ts`:
- Around line 48-53: Update toggleCallout in addCommands so an active callout
with a different type uses commands.updateAttributes(this.name, {type}) instead
of nesting via wrapIn; retain commands.toggleWrap(this.name, {type}) for all
other cases.
In `@webapp/src/components/page_editor/floating_formatting_bar.module.scss`:
- Around line 6-9: Remove the empty line between the z-index declaration and the
width declaration in the floating formatting bar styles so the declarations are
contiguous and satisfy Stylelint.
In `@webapp/src/components/page_editor/page_byline.tsx`:
- Around line 22-27: Update the component’s missing-author branch around getUser
and the author null check to dispatch getMissingProfilesByIds([userId]) when
author is unavailable before returning null. Add useEffect to the React imports
and use it to trigger this profile-loading dispatch when userId or author state
requires it, while preserving the existing rendering behavior once the profile
is available.
In `@webapp/src/components/page_editor/page_editor.tsx`:
- Around line 154-156: Update the early-return paths in the publish and
save-draft flows around autosave.flush() to call setActionError before returning
when the flush resolves false, using the existing error message pattern. Ensure
saveDraftAndLeave also catches rejected flush promises and sets actionError so
the exit dialog reports the failure instead of leaving an unhandled rejection.
- Around line 88-92: Update the page-change reset effect keyed by spaceId and
pageId to also clear conflict and showExitDialog, ensuring any publish-conflict
or exit dialog closes before actions can target the newly selected page.
- Around line 147-209: Replace the callback-captured busy guard in publish,
discard, and saveDraftAndLeave with a shared synchronous in-flight ref that is
checked and set before any await, then cleared when each action finishes.
Continue updating the existing busy state for rendering, and ensure all early
returns and finally paths release the ref.
In `@webapp/src/components/page_editor/toolbar_controls.tsx`:
- Around line 86-90: Guard the toolbar_controls.tsx insert callback so it
verifies the editor chain provides toggleCallout before invoking it, while
preserving focus, execution, and menu-closing behavior when supported. In
webapp/src/webapp_globals.ts lines 82-87, document the host build that
introduced contentType, extensions, and onContentError, or add a JSON-content
capability probe that page_editor.tsx can use for the legacy notice instead of
relying solely on hostSupportsDocumentEditor/getEditor.
In `@webapp/src/hooks/caret_anchored_suggestions.ts`:
- Around line 85-95: Update the effect around the MutationObserver in the
caret-anchored suggestions hook to schedule repositioning when the editor’s
data-docs-scroll container scrolls and when the window resizes. Register both
listeners while enabled, reuse the existing schedule callback, and remove them
in the cleanup alongside the observer and selectionchange listener.
- Around line 30-38: Update the caret-anchored positioning logic to clamp the
computed left offset before assigning `list.style.left`, using the surface width
and suggestion-list width so the list remains within the surface. Follow the
existing `maxLeft` clamping approach in `floating_formatting_bar.tsx` while
preserving the current caret-relative positioning when it fits.
In `@webapp/src/hooks/draft_autosave.ts`:
- Around line 140-143: Update the autosave flow around the Pending type, write,
and error-requeue logic to store baseEditAt alongside each queued patch, using
the value captured for that page rather than reading latest.current during
cleanup. Preserve the stored baseEditAt when requeueing failed patches, and add
coverage that switches between pages with different baselines and verifies each
patch uses its own baseline.
In `@webapp/src/hooks/page_draft.ts`:
- Around line 70-71: Update the draft field initialization in the page draft
hook to use nullish coalescing for both title and body, preserving empty-string
values while still falling back to page values only when the draft fields are
nullish.
In `@webapp/src/hooks/page_presence.ts`:
- Around line 54-57: Update the expired-snapshot branch in the page presence
effect to refresh the `now` state before exiting, rather than returning with the
mount-time value. Keep the existing timer scheduling for unexpired snapshots
unchanged so the memoized active-editor calculation receives the current
timestamp and clears stale editors.
---
Outside diff comments:
In `@webapp/src/components/page_editor/page_editor.module.scss`:
- Around line 1-10: Remove the empty line between the min-width and min-height
declarations in the .root style rule, leaving the declaration order and values
unchanged.
---
Nitpick comments:
In `@webapp/src/client/presence_events.ts`:
- Around line 21-23: Update publishPagePresence so each listener invocation is
isolated with per-listener error handling, ensuring an exception from one
subscriber does not stop iteration or propagate into the WebSocket handler;
preserve delivery of the event to all remaining listeners.
In `@webapp/src/components/page_editor/page_editor.module.scss`:
- Around line 28-32: Consolidate the three `.column` declarations into one
selector block, combining the layout, editor-surface, and callout rules while
preserving all existing properties and responsive behavior.
In `@webapp/src/components/page_editor/publish_conflict_dialog.tsx`:
- Around line 17-23: Update isConcurrentAutosave to accept the server’s
reason-code union rather than a generic string, and compare the value exactly
with === 'concurrent_autosave'. Reuse the existing reason-code type or define
the union from the server/draft-client reason values, preserving the current
explanatory-paragraph behavior for the exact concurrent_autosave code.
In `@webapp/src/components/page_editor/toolbar_controls.tsx`:
- Around line 138-146: Move CALLOUT_LABELS above CalloutControl and change its
values from formatter functions to MessageDescriptor objects. Remove the
Formatter indirection, and update CalloutControl to pass CALLOUT_LABELS[type]
directly to formatMessage when rendering the label.
- Around line 112-133: Align the callout menu’s accessibility semantics with its
behavior in the toolbar component: either implement arrow-key navigation and
roving tabindex for the `menu` and `menuitem` elements around
`CALLOUT_TYPES.map`, or remove those role attributes so the controls remain a
plain button group. Preserve the existing `insert(type)` activation behavior.
In `@webapp/src/hooks/caret_anchored_suggestions.ts`:
- Around line 6-8: Add a concise comment next to the SELECTOR constant
documenting the host version associated with the `.suggestion-list` class,
preserving the existing selector and positioning behavior.
In `@webapp/src/hooks/draft_autosave.test.tsx`:
- Around line 266-280: Expand the page-change coverage around setup and the
“flushes the pending patch…” test to provide an initial baseEditAt, rerender
with both a new pageId and different baseEditAt, and assert the flushed page1
patch includes the previous page’s base_edit_at. Add a separate case using
baseEditAt: 0 and verify the emitted patch preserves base_edit_at: 0.
In `@webapp/src/hooks/draft_autosave.ts`:
- Around line 112-124: The queue function’s debounce can be postponed
indefinitely during continuous edits. Add tracking for when the current pending
batch first starts, and in queue enforce a maximum wait interval that triggers
write even when the debounce timer keeps resetting; reset that tracking when the
pending changes are written or cleared, while preserving the existing debounce
behavior for shorter pauses.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b73604e5-f7a9-4c16-9257-acc82cd46454
⛔ Files ignored due to path filters (1)
webapp/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (37)
webapp/i18n/en.jsonwebapp/package.jsonwebapp/src/client/drafts.tswebapp/src/client/pages.tswebapp/src/client/presence_events.tswebapp/src/client/rest.tswebapp/src/components/docs_root/docs_main_content.tsxwebapp/src/components/page_editor/apply_formatting.tswebapp/src/components/page_editor/autosave_indicator.module.scsswebapp/src/components/page_editor/autosave_indicator.tsxwebapp/src/components/page_editor/callout_extension.tswebapp/src/components/page_editor/docs_extensions.tswebapp/src/components/page_editor/exit_editor_dialog.module.scsswebapp/src/components/page_editor/exit_editor_dialog.tsxwebapp/src/components/page_editor/floating_formatting_bar.module.scsswebapp/src/components/page_editor/floating_formatting_bar.tsxwebapp/src/components/page_editor/page_byline.module.scsswebapp/src/components/page_editor/page_byline.tsxwebapp/src/components/page_editor/page_editor.module.scsswebapp/src/components/page_editor/page_editor.tsxwebapp/src/components/page_editor/publish_conflict_dialog.module.scsswebapp/src/components/page_editor/publish_conflict_dialog.tsxwebapp/src/components/page_editor/toolbar_controls.module.scsswebapp/src/components/page_editor/toolbar_controls.tsxwebapp/src/data/fixtures.tswebapp/src/hooks/caret_anchored_suggestions.tswebapp/src/hooks/draft_autosave.test.tsxwebapp/src/hooks/draft_autosave.tswebapp/src/hooks/page_draft.tswebapp/src/hooks/page_presence.tswebapp/src/hooks/pinned_toolbar.tswebapp/src/hooks/user.tswebapp/src/index.tsxwebapp/src/store/test_fixtures.tswebapp/src/types/docs.tswebapp/src/types/drafts.tswebapp/src/webapp_globals.ts
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThe PR adds a Docs page editor with Tiptap formatting, callouts, floating and pinned toolbars, draft autosave, presence tracking, publishing safeguards, localized states, and updated editor styling. ChangesDocs editor
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
webapp/src/hooks/page_draft.ts (1)
73-74: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve empty draft fields with nullish coalescing.
Draft.titleandDraft.bodyare required strings, and autosave sends empty strings. With||, a cleared title or a cleared body falls back to the published page content on the next load. The user then sees content that the draft no longer contains.Use
??so the fallback applies only when the draft field isnullorundefined.🐛 Proposed fix to preserve cleared draft fields
- title: draft?.title || page?.title || '', - body: draft?.body || page?.body || '', + title: draft?.title ?? page?.title ?? '', + body: draft?.body ?? page?.body ?? '',🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webapp/src/hooks/page_draft.ts` around lines 73 - 74, Update the draft initialization around title and body to use nullish coalescing instead of falsy coalescing, so empty-string Draft.title and Draft.body values are preserved while fallback to page values occurs only for null or undefined.webapp/src/components/page_editor/page_editor.tsx (1)
159-162: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winA failed
autosave.flush()still produces no user-visible message.Line 160 and Line 211 return early when
flush()resolvesfalse. Neither path callssetActionError. Thefinallyblock clearsbusy, so the button becomes active and nothing else changes. The user sees no result and no reason.
saveDraftAndLeaveat Line 203 also has nocatch. Ifflush()rejects, then the rejection is unhandled and the exit dialog shows no error, becausefailed={actionError != null}staysfalseat Line 418.🐛 Proposed fix
try { if (!await autosave.flush()) { + setActionError(new Error('autosave_flush_failed')); return; }Apply the same change at Line 211, and add a
catchblock tosaveDraftAndLeavethat callssetActionError(error).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webapp/src/components/page_editor/page_editor.tsx` around lines 159 - 162, Update both autosave.flush() call paths to surface failures through setActionError: when flush() resolves false, set the action error before returning, and add a catch block to saveDraftAndLeave that passes the rejected error to setActionError. Preserve the existing finally cleanup and successful save/navigation behavior.
🧹 Nitpick comments (9)
webapp/src/hooks/draft_autosave.test.tsx (1)
172-189: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd an assertion for recovery after a failed save.
The test verifies the retry patch content, but it does not verify that the status returns to
savedafter the retry succeeds. That transition drives the autosave indicator inpage_editor.tsx.Add the assertion at the end of the test.
♻️ Proposed additional assertion
await act(async () => { await result.current.flush(); }); expect(patchesSent()[1]).toEqual({body: 'lost'}); + expect(result.current.status).toBe('saved');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webapp/src/hooks/draft_autosave.test.tsx` around lines 172 - 189, Add an assertion at the end of the “keeps the patch for retry when a save fails” test verifying that result.current.status transitions to “saved” after flush() successfully retries the preserved patch.webapp/src/hooks/pinned_toolbar.ts (2)
6-6: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winScope the storage key to the user.
STORAGE_KEYis a single global key. If two accounts use the same browser profile, they share one toolbar preference. The PR description lists the toolbar preference storage as an open question, so this choice is worth confirming now.Two options exist:
- Append the current user id to the key.
useCurrentUserIdinwebapp/src/hooks/user.tsalready provides the id.- Store the preference as a Mattermost user preference, so it follows the user across devices.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webapp/src/hooks/pinned_toolbar.ts` at line 6, Update the pinned-toolbar persistence around STORAGE_KEY to scope the stored preference to the authenticated user, using the existing useCurrentUserId symbol from the user hook when constructing the key. Ensure different users receive separate storage entries and preserve the existing toolbar preference behavior.
30-37: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
writeStoredreturns a value that no caller uses.
writeStoredreturnstrueorfalse, but line 36 discards the result. A failed write is therefore silent, and the toolbar state and the stored state diverge without any signal.Either drop the return type, or use the result to surface the failure.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webapp/src/hooks/pinned_toolbar.ts` around lines 30 - 37, Update the pinned toolbar effect around writeStored so its boolean result is no longer silently discarded: either remove the unused return value from writeStored or handle false by surfacing the storage failure, while preserving the first-render skip behavior.webapp/src/client/presence_events.ts (1)
21-23: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winIsolate listener failures during dispatch.
If one listener throws,
publishPagePresencestops and the exception propagates to the caller. The caller is the WebSocket handler registered inwebapp/src/index.tsx, so a single failing subscriber can affect host event dispatch and can block the remaining subscribers.Wrap each invocation so that one failure does not stop the others.
♻️ Proposed fix to isolate listener errors
export function publishPagePresence(event: PagePresenceEvent): void { - listeners.forEach((listener) => listener(event)); + listeners.forEach((listener) => { + try { + listener(event); + } catch { + // A failing subscriber must not block the remaining subscribers. + } + }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webapp/src/client/presence_events.ts` around lines 21 - 23, Update publishPagePresence to invoke each listener within its own failure boundary, ensuring an exception from one listener is contained and does not propagate to the WebSocket caller or prevent remaining listeners from receiving the event.webapp/src/components/page_editor/exit_editor_dialog.module.scss (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd the copyright header.
floating_formatting_bar.module.scssandtoolbar_controls.module.scssin the same directory both start with the two-line Mattermost copyright comment. This file does not.♻️ Proposed addition
+// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + .actions { display: flex;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webapp/src/components/page_editor/exit_editor_dialog.module.scss` at line 1, Add the standard two-line Mattermost copyright comment at the beginning of the stylesheet containing the .actions rule, matching the header used by floating_formatting_bar.module.scss and toolbar_controls.module.scss.webapp/src/components/page_editor/toolbar_controls.module.scss (1)
66-71: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
.swatchclass.
toolbar_controls.tsxusescontrol,active,menuWrapper,menu, andmenuItem. It does not useswatch. The menu items render Compass icons, not colour swatches.♻️ Proposed removal
- -.swatch { - width: 12px; - height: 12px; - border-radius: 2px; - flex-shrink: 0; -}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webapp/src/components/page_editor/toolbar_controls.module.scss` around lines 66 - 71, Remove the unused .swatch style rule from the toolbar controls stylesheet; retain the styles for the classes used by toolbar_controls.tsx, including control, active, menuWrapper, menu, and menuItem.webapp/src/components/page_editor/floating_formatting_bar.tsx (1)
85-100: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winThe scroll listener attaches only if
editorRef.currentis set on the first effect run.Line 86 resolves the scroll container once. The effect depends on
[schedule, editorRef].editorRefis a stable ref object, so the effect does not re-run wheneditorRef.currentchanges later. If the referenced element mounts after this effect runs,scrollerstaysnulland the bar never repositions on scroll.Bind the scroll listener at the document level with capture, which does not depend on resolving the container.
♻️ Proposed change
useEffect(() => { - const scroller = editorRef.current?.closest('[data-docs-scroll]'); - document.addEventListener('selectionchange', schedule); window.addEventListener('resize', schedule); - scroller?.addEventListener('scroll', schedule); + document.addEventListener('scroll', schedule, true); return () => { document.removeEventListener('selectionchange', schedule); window.removeEventListener('resize', schedule); - scroller?.removeEventListener('scroll', schedule); + document.removeEventListener('scroll', schedule, true); if (frameRef.current) { cancelAnimationFrame(frameRef.current); frameRef.current = 0; } }; }, [schedule, editorRef]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webapp/src/components/page_editor/floating_formatting_bar.tsx` around lines 85 - 100, Update the scroll handling in the useEffect so it no longer resolves a container from editorRef.current or attaches to scroller; register the scroll listener on document with capture enabled and remove it using the same capture configuration during cleanup, while preserving the existing selection, resize, and animation-frame cleanup behavior.webapp/src/components/page_editor/page_editor.module.scss (1)
62-73: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMerge the duplicate
.columnselectors.
.columnis declared at Line 28, Line 62, and Line 88. Three separate blocks for the same class make the cascade harder to follow. Merge the two:globalblocks into one block under a single.columnrule.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webapp/src/components/page_editor/page_editor.module.scss` around lines 62 - 73, Merge the duplicate .column selectors in the stylesheet into one rule, combining both :global blocks under it while preserving all existing declarations and cascade behavior. Use the existing .column rule as the single location for these styles.webapp/src/components/page_editor/publish_conflict_dialog.tsx (1)
23-27: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueCompare the conflict reason exactly instead of using a substring match.
reasonis a discrete server reason code.includesmatches any future code that embeds theconcurrent_autosavetoken and selects the wrong message. Use an exact comparison against a shared constant.♻️ Proposed change
-const isConcurrentAutosave = (reason: string): boolean => reason.includes('concurrent_autosave'); +const CONCURRENT_AUTOSAVE = 'concurrent_autosave'; + +const isConcurrentAutosave = (reason: string): boolean => reason === CONCURRENT_AUTOSAVE;Export the constant from
client/draftsif the reason codes are already defined there.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webapp/src/components/page_editor/publish_conflict_dialog.tsx` around lines 23 - 27, Update isConcurrentAutosave to compare reason by exact equality with the shared concurrent-autosave reason constant, reusing and exporting that constant from client/drafts if the reason codes are defined there; remove the substring-based includes check while preserving the existing autosave conflict selection.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@webapp/i18n/en.json`:
- Around line 27-31: Update the user-facing strings in the English translations
so docs.editor.autosave.saving, docs.editor.bodyPlaceholder, and the
corresponding string at line 49 use the same ellipsis convention, and change
docs.editor.callout to use “Callout” as the noun while preserving the existing
meaning.
In `@webapp/src/components/page_editor/page_editor.module.scss`:
- Around line 6-7: Remove the blank line immediately before the min-height
declaration in the SCSS file. Update the Stylelint configuration used for CSS
Modules files to ignore the global pseudo-class, reusing or extending the
existing .module.scss-specific rules after confirming whether other module
styles are already covered.
In `@webapp/src/components/page_editor/page_editor.tsx`:
- Around line 394-406: Update the WysiwygEditor usage in the page editor to set
useCtrlSend={true}, ensuring onPublish is triggered only through the intended
modified-key shortcut rather than an unmodified Enter in a paragraph.
In `@webapp/src/components/page_editor/publish_conflict_dialog.tsx`:
- Around line 39-54: The force-publish action in the conflict dialog lacks
loading and failure feedback. Update the dialog component’s props and the
`PrimaryButton` using `onForcePublish` to accept and mirror the `busy` and
`failed` handling used by `ExitEditorDialog`, so failed forced publishes are
surfaced within the open dialog.
In `@webapp/src/components/page_editor/toolbar_controls.tsx`:
- Around line 114-135: Update the menu item selection handler in the
CALLOUT_TYPES map to restore focus to triggerRef after insert(type) closes the
menu, unless the successful editor command has already moved focus to the editor
surface. Match the existing Escape-path focus behavior while preserving the
insert command flow.
---
Duplicate comments:
In `@webapp/src/components/page_editor/page_editor.tsx`:
- Around line 159-162: Update both autosave.flush() call paths to surface
failures through setActionError: when flush() resolves false, set the action
error before returning, and add a catch block to saveDraftAndLeave that passes
the rejected error to setActionError. Preserve the existing finally cleanup and
successful save/navigation behavior.
In `@webapp/src/hooks/page_draft.ts`:
- Around line 73-74: Update the draft initialization around title and body to
use nullish coalescing instead of falsy coalescing, so empty-string Draft.title
and Draft.body values are preserved while fallback to page values occurs only
for null or undefined.
---
Nitpick comments:
In `@webapp/src/client/presence_events.ts`:
- Around line 21-23: Update publishPagePresence to invoke each listener within
its own failure boundary, ensuring an exception from one listener is contained
and does not propagate to the WebSocket caller or prevent remaining listeners
from receiving the event.
In `@webapp/src/components/page_editor/exit_editor_dialog.module.scss`:
- Line 1: Add the standard two-line Mattermost copyright comment at the
beginning of the stylesheet containing the .actions rule, matching the header
used by floating_formatting_bar.module.scss and toolbar_controls.module.scss.
In `@webapp/src/components/page_editor/floating_formatting_bar.tsx`:
- Around line 85-100: Update the scroll handling in the useEffect so it no
longer resolves a container from editorRef.current or attaches to scroller;
register the scroll listener on document with capture enabled and remove it
using the same capture configuration during cleanup, while preserving the
existing selection, resize, and animation-frame cleanup behavior.
In `@webapp/src/components/page_editor/page_editor.module.scss`:
- Around line 62-73: Merge the duplicate .column selectors in the stylesheet
into one rule, combining both :global blocks under it while preserving all
existing declarations and cascade behavior. Use the existing .column rule as the
single location for these styles.
In `@webapp/src/components/page_editor/publish_conflict_dialog.tsx`:
- Around line 23-27: Update isConcurrentAutosave to compare reason by exact
equality with the shared concurrent-autosave reason constant, reusing and
exporting that constant from client/drafts if the reason codes are defined
there; remove the substring-based includes check while preserving the existing
autosave conflict selection.
In `@webapp/src/components/page_editor/toolbar_controls.module.scss`:
- Around line 66-71: Remove the unused .swatch style rule from the toolbar
controls stylesheet; retain the styles for the classes used by
toolbar_controls.tsx, including control, active, menuWrapper, menu, and
menuItem.
In `@webapp/src/hooks/draft_autosave.test.tsx`:
- Around line 172-189: Add an assertion at the end of the “keeps the patch for
retry when a save fails” test verifying that result.current.status transitions
to “saved” after flush() successfully retries the preserved patch.
In `@webapp/src/hooks/pinned_toolbar.ts`:
- Line 6: Update the pinned-toolbar persistence around STORAGE_KEY to scope the
stored preference to the authenticated user, using the existing useCurrentUserId
symbol from the user hook when constructing the key. Ensure different users
receive separate storage entries and preserve the existing toolbar preference
behavior.
- Around line 30-37: Update the pinned toolbar effect around writeStored so its
boolean result is no longer silently discarded: either remove the unused return
value from writeStored or handle false by surfacing the storage failure, while
preserving the first-render skip behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d019f109-4109-4213-a227-e285f66c7f60
⛔ Files ignored due to path filters (1)
webapp/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (37)
webapp/i18n/en.jsonwebapp/package.jsonwebapp/src/client/drafts.tswebapp/src/client/pages.tswebapp/src/client/presence_events.tswebapp/src/client/rest.tswebapp/src/components/docs_root/docs_main_content.tsxwebapp/src/components/page_editor/apply_formatting.tswebapp/src/components/page_editor/autosave_indicator.module.scsswebapp/src/components/page_editor/autosave_indicator.tsxwebapp/src/components/page_editor/callout_extension.tswebapp/src/components/page_editor/docs_extensions.tswebapp/src/components/page_editor/exit_editor_dialog.module.scsswebapp/src/components/page_editor/exit_editor_dialog.tsxwebapp/src/components/page_editor/floating_formatting_bar.module.scsswebapp/src/components/page_editor/floating_formatting_bar.tsxwebapp/src/components/page_editor/page_byline.module.scsswebapp/src/components/page_editor/page_byline.tsxwebapp/src/components/page_editor/page_editor.module.scsswebapp/src/components/page_editor/page_editor.tsxwebapp/src/components/page_editor/publish_conflict_dialog.module.scsswebapp/src/components/page_editor/publish_conflict_dialog.tsxwebapp/src/components/page_editor/toolbar_controls.module.scsswebapp/src/components/page_editor/toolbar_controls.tsxwebapp/src/data/fixtures.tswebapp/src/hooks/caret_anchored_suggestions.tswebapp/src/hooks/draft_autosave.test.tsxwebapp/src/hooks/draft_autosave.tswebapp/src/hooks/page_draft.tswebapp/src/hooks/page_presence.tswebapp/src/hooks/pinned_toolbar.tswebapp/src/hooks/user.tswebapp/src/index.tsxwebapp/src/store/test_fixtures.tswebapp/src/types/docs.tswebapp/src/types/drafts.tswebapp/src/webapp_globals.ts
Collaborator
|
@asaadmahmood - for (some of) the "Open Questions" in #13 (comment) |
jgheithcock
reviewed
Aug 3, 2026
jgheithcock
reviewed
Aug 3, 2026
jgheithcock
reviewed
Aug 3, 2026
jgheithcock
reviewed
Aug 3, 2026
jgheithcock
requested changes
Aug 3, 2026
calebroseland
added a commit
that referenced
this pull request
Aug 3, 2026
Pre-resolves the parts of the PR #13 (MM-69893) merge that are worth keeping whichever order the two PRs land in, so the editor branch can be merged, reverted and re-merged without redoing them. - webapp_globals: adopt the document-mode editor contract — contentType, extensions, onContentError, plus getEditor/hasContentError on the handle and a hostSupportsDocumentEditor probe. A Docs page body is structured content, so the editor is useless to us in markdown mode. Also onChange's param rename and ReactNodeArray -> ReactNode[], which drops a deprecation warning. - rest: thrown errors become RestError, a ClientError subclass that keeps the parsed payload and the server error id. Draft publish answers a 409 with the current page in the body, which ClientError alone discards. Existing instanceof ClientError / status_code callers are unaffected. - rest: add a path-relative doFetch over the same request path, for client modules that describe a whole endpoint family. - types: drop the stale note on Page — user_id and last_modified_by are here. The editor branch's client layer compiles against this without edits, so it no longer needs its own transport (which hand-rolled headers and relied on the deprecated XMLHttpRequest CSRF fallback). Prompt: merge PR#13 for preview with a revert-marked commit; conflict resolution in a separate commit that stays, to reduce conflicts when both PRs merge
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
webapp/src/components/page_editor/page_editor.tsx (1)
153-183: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winIgnore action results after the route changes.
If a publish for page A remains in flight after navigation to page B, Line 175 can open page A's conflict dialog on page B. The dialog then calls the current
publishcallback and force-publishes page B.
discardandsaveDraftAndLeavecan also callleave()for page A after navigation to page B.Capture a route generation or immutable page key when each action starts. Before each post-await state update, navigation, or conflict update, return when that key no longer matches the current route.
Proposed guard
+ const routeKeyRef = useRef(`${spaceId}:${pageId}`); + useEffect(() => { + routeKeyRef.current = `${spaceId}:${pageId}`; setContentError(false); // ... }, [spaceId, pageId]); const publish = useCallback(async (force: boolean, exitAfter = false) => { + const actionRouteKey = `${spaceId}:${pageId}`; // ... try { if (!await autosave.flush()) { return; } + if (routeKeyRef.current !== actionRouteKey) { + return; + } const published = await publishPageDraft(spaceId, pageId, force); + if (routeKeyRef.current !== actionRouteKey) { + return; + } // state updates and leave()Also applies to: 185-222
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webapp/src/components/page_editor/page_editor.tsx` around lines 153 - 183, Guard publish, discard, and saveDraftAndLeave against stale results after navigation by capturing the current route generation or immutable page key when each action begins. Before every post-await state update, conflict/dialog update, or leave() call, verify the captured key still matches the current route and return otherwise; ensure stale page A actions cannot affect page B.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@webapp/src/components/page_editor/page_editor.tsx`:
- Around line 153-183: Guard publish, discard, and saveDraftAndLeave against
stale results after navigation by capturing the current route generation or
immutable page key when each action begins. Before every post-await state
update, conflict/dialog update, or leave() call, verify the captured key still
matches the current route and return otherwise; ensure stale page A actions
cannot affect page B.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4d83bd77-8081-44df-b8b8-f242e7a2d7c4
📒 Files selected for processing (13)
webapp/i18n/en.jsonwebapp/src/components/page_editor/apply_formatting.tswebapp/src/components/page_editor/autosave_indicator.tsxwebapp/src/components/page_editor/floating_formatting_bar.module.scsswebapp/src/components/page_editor/floating_formatting_bar.tsxwebapp/src/components/page_editor/page_editor.tsxwebapp/src/components/page_editor/publish_conflict_dialog.module.scsswebapp/src/components/page_editor/publish_conflict_dialog.tsxwebapp/src/components/page_editor/toolbar_controls.tsxwebapp/src/hooks/draft_autosave.test.tsxwebapp/src/hooks/draft_autosave.tswebapp/src/hooks/page_draft.test.tsxwebapp/src/hooks/page_draft.ts
💤 Files with no reviewable changes (1)
- webapp/src/components/page_editor/floating_formatting_bar.module.scss
🚧 Files skipped from review as they are similar to previous changes (9)
- webapp/src/components/page_editor/publish_conflict_dialog.module.scss
- webapp/src/components/page_editor/autosave_indicator.tsx
- webapp/src/hooks/page_draft.ts
- webapp/i18n/en.json
- webapp/src/components/page_editor/toolbar_controls.tsx
- webapp/src/components/page_editor/floating_formatting_bar.tsx
- webapp/src/hooks/draft_autosave.test.tsx
- webapp/src/hooks/draft_autosave.ts
- webapp/src/components/page_editor/apply_formatting.ts
nang2049
force-pushed
the
MM-69893-page-editor-autosave
branch
from
August 5, 2026 14:54
3a894db to
b08e925
Compare
calebroseland
added a commit
that referenced
this pull request
Aug 5, 2026
Drafts as a first-class slice, ported from PR #13's client onto this branch's data-source seam so there is one transport rather than two. - types/drafts.ts: Draft, DraftSummary, DraftPatch, publish conflict shapes - DocsDataSource gains create/get/update/delete/list/publish for drafts; a missing draft normalizes to undefined rather than a 404 the caller catches - PublishConflictError unpacks publish's 409, which uniquely carries data (the current server page), and distinguishes forceable conflicts from an unpublished parent, which force cannot fix - drafts + draftsInSpace reducers, keyed by page id and kept separate from pages so one user's unpublished title cannot render in another's tree - PUBLISHED_DRAFT is one action, removing the draft and adding the page together, so no render shows the new page twice - selectors distinguish unpublished edits to a page from an orphan draft (an unpublished page), which is the duplicate-row trap encoded once Thunks cover fetch/create/save/discard/publish. No UI consumes these yet. Prompt: Back to Drafts DAL/Reducers/Selectors & interop/integration with Page Tree and Pages DAL/hooks
nang2049
force-pushed
the
MM-69893-page-editor-autosave
branch
from
August 6, 2026 09:00
b08e925 to
0052d7d
Compare
nang2049
marked this pull request as ready for review
August 6, 2026 09:01
jgheithcock
approved these changes
Aug 6, 2026
jgheithcock
left a comment
jgheithcock
left a comment
Collaborator
There was a problem hiding this comment.
There were a number of issues that AI uncovered, but after a discussion with Caleb, these should all go away when you integrate more fully with Caleb's PR #12. (They were all connected to what I now understand you meant by POC - scaffolding you needed up until you were connected into the larger UI framework). So LGTM!
nang2049
force-pushed
the
MM-69893-page-editor-autosave
branch
from
August 10, 2026 06:07
0052d7d to
c58bcb1
Compare
Contributor
Author
|
@asaadmahmood kind reminder for UX comments |
calebroseland
requested changes
Aug 11, 2026
calebroseland
left a comment
calebroseland
left a comment
Member
There was a problem hiding this comment.
Thanks for all your work on this so far. It's really cool to see editable content and real data getting to the DB!
I've commented on some things, but it seems there's still a good amount of migration/rewiring after the rebase, so I haven't gone very deep on the review.
I'm trying to get #12 ready to merge--I believe it's mostly done, just trying to work though final touches and review items into a good-enough state. There will be minor things to clean up on, but I think we should try to get what we have aligned and merged, so we can better work off of master and demo the progress we're making.
| exitAfter: boolean; | ||
| }; | ||
|
|
||
| const PageEditor = ({spaceId, pageId, isDraft}: Props) => { |
Member
There was a problem hiding this comment.
PageEditor contains flat component level infra that I think should be moved to smaller custom hooks, reusable actions/store, and/or more integrated with DAL infra. I know you're probably still working on migrating some of these over as part of the final rebase, so I won't drill down into too many of the specifics-- just wanting to make sure we get to it at some point before merging.
as an example, sometimes it doesn't appear to load or has trouble reading published pages (or perhaps it assumes it's a draft?)
| failed?: boolean; | ||
| }; | ||
|
|
||
| const ExitEditorDialog = ({onPublish, onSaveDraft, onDiscard, onClose, busy = false, failed = false}: Props) => { |
Member
There was a problem hiding this comment.
Could we integrate this or migrate/merge with the edit mode and publish flows that are connected to the store infra/DAL? Also, this modal doesn't appear to know or take into consideration if the page already published.
Comment on lines
+291
to
+313
| <AutosaveIndicator status={autosave.status}/> | ||
| <PrimaryButton | ||
| onClick={onPublish} | ||
| disabled={busy || contentError} | ||
| > | ||
| {load.page ? ( | ||
| <FormattedMessage | ||
| id='docs.editor.update' | ||
| defaultMessage='Update' | ||
| /> | ||
| ) : ( | ||
| <FormattedMessage | ||
| id='docs.editor.header.published' | ||
| defaultMessage='Published · {spaceId} / {pageId}' | ||
| values={{spaceId, pageId}} | ||
| id='docs.editor.publish' | ||
| defaultMessage='Publish' | ||
| /> | ||
| )} | ||
| </span> | ||
| </PrimaryButton> | ||
| <SecondaryButton onClick={onClose}> | ||
| <FormattedMessage | ||
| id='docs.editor.close' | ||
| defaultMessage='Close' | ||
| /> | ||
| </SecondaryButton> |
Member
There was a problem hiding this comment.
Could we move these up to PageHeader and merge/integrate with the edit/publish flows that are connected to the store infra/DAL?
Contributor
Author
|
Thanks @calebroseland pushed the rewiring from your review. Draft load, autosave and publish all dispatch through the DAL now. Deleted the plugin's client/drafts.ts / client/pages.ts and my own publish path and conflict dialog and publishing runs through your usePublishDraft. Publish and Close are gone from the editor. save status and presence moved into PageHeader. I kept a thin useEditorContent rather than your usePageDraft because the editor needs the page body and has to open on a snapshot. |
calebroseland
added a commit
that referenced
this pull request
Aug 11, 2026
* chore(docs-editor): integrate stub page editor from PR #7
Soft-merge of MM-69892 (PR #7, nang2049): the editor slice in
webapp_globals and a stub PageEditor mounted at the page/draft routes,
so the spaces-API wiring can build around the editor and avoid later
conflicts.
Applied as a net diff onto master since PR #7 was cut from the pre-merge
scaffolding branch. Two divergences resolved:
- Dropped PR #7's navigation.ts change: master's merged draft-route fix
already exposes the isDraft the editor consumes (single
[DOCS_DRAFT_ROUTE, DOCS_ROUTE] match instead of two useRouteMatch calls).
- webapp_globals: took PR #7's superset React type imports; kept master's
useBootstrapDocs in docs_root (3-way merged cleanly with the isDraft add).
* feat(docs): wire spaces and membership to the plugin REST API
Replace the mock data source with an API-backed DocsDataSource over the
plugin's /api/v1 routes (server/api.go), following the Playbooks
fetch(url, Client4.getOptions()) idiom. Spaces are created, listed
(team-scoped by backing-channel membership), and left against the real
server; the switcher fans out per team for cross-team results.
- client/rest: doGet/doPost/doDelete + paginated listAll ({items,has_more})
- data: async DocsDataSource + apiDataSource; remove mock fixtures
- store: fetchSpaces/fetchAllSpaces/createSpace/leaveSpace async thunks;
drop the team_id hashing stand-in (the server scopes lists by team)
- opaque server ids: drop slug-as-id and isSlugAvailable; the slug stays a
client-only vanity field, format-validated only
- recent: client-side view history (data/recent_spaces) as the seam for a
later server last_viewed_at proxy; page count omitted for MVP
- leave space: wired to member removal, navigates home when viewing it
- visibility stays client-only, maps to server view_access later (PR #10)
Addresses PR #2 feedback (team_id hashing, slug assumption, leaveSpace
wiring). Targets current master; forward-compatible with PR #10.
* chore(i18n): re-extract en.json after spaces-API wiring
Adds the docs.editor.* ids from the stub editor and drops the removed
url.taken and page-placeholder ids plus the stale favorites ids.
* refactor(docs): drop the vestigial create-space URL field
The slug/URL field became dead weight once spaces route by opaque server
id: it was collected and format-validated but never sent or used. Remove
it from the create-space modal and the form/schema — slug is gone from
CreateSpaceInput, the form values, and the Zod schema (with its Url*
errors and slugify helper). The UrlInput form-control component and its
test are kept for reuse. Re-extract en.json (drops the url.* messages).
* feat(docs): generic space-icon fallback and switcher recent polish
Custom emoji/icon picking is deferred, so a space without a custom icon
renders a generic compass glyph (utils/space_icon: <SpaceIcon>, using the
product's file-text-outline) everywhere a space icon shows — sidebar, home
cards, main content, switcher, and the create-space name field — so a
freshly created space never appears blank. A space with a custom emoji
renders that character.
Switcher: rename the "Recent docs" group to "Recent", and dedupe — a space
already listed under Recent is no longer repeated under "Your spaces".
* revert(docs-editor): remove non-working stub page editor
Reverts 13b2db3 (soft-merge of PR #7's stub editor): the mounted editor
wasn't working. Drops the page_editor component, the editor slice from
webapp_globals, and the pageId->PageEditor routing in docs_main_content
(back to the space/page placeholder). Later work on the touched files
(SpaceIcon, useRecordSpaceView) is preserved. Re-extract en.json.
* feat(docs): build the Space Home main content UI
Implements the space main-content view (Figma Space Home) as a new
space_view component set, replacing the routed-space placeholder:
- SpaceTitleBar: favorite, space icon + title + menu, member count, space
details, Share
- PageBar: page-tree toggle + "Pages", "Updated <relative>" (host
Timestamp), comments, Edit, overflow, expand
- PageHero: rounded banner with the space icon/title, description (with a
placeholder when empty), and a pages/members/views stats row
- Page body: a placeholder until the editor is mounted
Uses Mattermost theme tokens and compass icons. Data-less bits are honest
placeholders for now: the stat counts and title-bar member count render an
em dash (await the pages/members/views APIs), member avatars are omitted,
and the controls are visual scaffolding wired in later passes.
* feat(docs): wire space page and member count pipelines
Feeds the Space Home stats and title-bar member count with real data.
- data: add listSpaceMembers over GET /spaces/{id}/members; normalize the
page-list summaries (no body) to Page for the store
- store: new spaceMembers slice (user ids per space) + fetchSpaceMembers;
fetchPages now stores pages for the count (reused by the page tree later)
- selectors: getSpaceMemberIds; page count derives from getPagesForSpace
- hooks: useSpaceStats loads both on space mount and returns the counts
- space view: real PAGES and MEMBERS counts in the hero and the title-bar
member count
Views has no server source yet, so that stat stays an em dash.
* feat(docs): member avatars, Share modal, design-system buttons
Space view build-out plus a raw-<button> cleanup.
- buttons: replace the raw <button>s in the space title/page bars with the
shared Button — icon buttons use emphasis="quaternary" + the compass
`btn-icon` class (the core pattern), Edit uses SecondaryButton
- avatars: expose the host Avatar via webapp_globals (as a wrapper that
no-ops when the host doesn't publish it, so callers need no null check);
useSpaceMemberProfiles resolves member ids to profiles (fetching missing
ones); MemberAvatars renders the overlapping stack + "+N" in the hero
- share: ShareSpaceModal — real member list with avatars/names and a
functional Copy link; the add-people search, per-member role, and space
visibility/access are scaffolding (capabilities + view_access land with
PR #10). Wired to the title-bar Share button.
* feat(docs): nested, collapsible, drag-and-drop page tree
Adds the space page-tree panel (Figma) as a middle column in the space
view, backed by the real page + move APIs.
- store: buildPageTree (parent_id/sort_order, orphans as roots) +
buildDescendantMap; reindexAfterMove (0-based sibling renumber mirroring
the server) + MOVED_PAGE; movePage thunk (optimistic move, reconcile on
success, re-fetch on failure) and createPage thunk
- data: DocsDataSource.movePage (PATCH .../move) and createPage (POST);
restPatch
- collapse: per-user localStorage tracker (data/collapsed_pages) +
useCollapsedPages, mirroring the recent-spaces seam
- ui: PageTreePanel (264px column: Pages header, add-page, tree) and the
recursive PageTreeNode (disclosure chevron, page/folder glyph, indent,
active highlight, navigate/collapse)
- dnd: pragmatic-drag-and-drop reused from spaces_sidebar — a 3-zone
hitbox (reorder above/below, reparent onto center), descendant-drop
guard, and a monitor resolving the drop into (parent_id, sibling_index)
- layout: space view is now the title bar over [tree panel][content
column]; the page-bar Pages button toggles the panel
- tests: buildPageTree/descendants and reindexAfterMove/MOVED_PAGE
Design's role/visibility remain PR #10 scaffolding; the page body is still
a placeholder until the editor is mounted.
* feat(docs): people-search combobox for Share, gated on canManageMembers
Adds a real debounced people-search pipeline and a Base UI Autocomplete
combobox for the Share modal, gated on a named permission so nothing
misleading ships.
There is no server add-member API yet (roles/view-access + capabilities
land with PR #10), so a live "add people" control could only hold
selections client-side. The picker renders only when canManageMembers.
- store/permissions: getSpacePermissions(state, spaceId) selector +
useSpacePermissions(spaceId) hook — the get/use split from core's
channel_bookmarks/utils — returning named SpacePermissions
{canManageMembers}; the seam PR #10's capability set drops into
- hooks/user_search: useUserSearch — 300ms-debounced searchProfiles scoped
to the current team, mapped to MemberProfile, excludes already-shown ids
- share_space_modal/people_picker: Base UI Autocomplete (server-driven
list, host Avatar rows, empty/searching states)
- share_space_modal: render the picker only when canManageMembers
* chore(docs): dev-ungate add-members; member count opens Share
- permissions: a DEV override forces canManageMembers on so the add-people
picker is exercisable before PR #10's capability set exists. The get/use
permission hooks are unchanged; revert the default to false (and wire the
real capability source) before shipping.
- space title bar: the member count is now a button that opens the Share
modal, the same as the Share button.
* fix(docs): neutral grey for btn-icon buttons (port-to-core)
The shared Button is accent-only: every emphasis (quaternary etc.) colors
with --button-bg, and its rule follows .btn-icon in core's _buttons.scss,
so `<Button emphasis='quaternary' className='btn-icon'>` renders the
canonical icon markup but in accent blue instead of neutral grey.
Add a :global override that raises .btn-icon's specificity over the
emphasis so icon buttons stay neutral (center-channel-color), loaded as a
side effect via the button wrapper so it applies everywhere. Marked
PORT-TO-CORE — fold into core's .btn-icon rule and delete when it lands.
* refactor(docs): neutral toolbar buttons; form-controls -> form_controls
Button styling:
- Pages toggle and Edit are neutral ghost buttons now (not accent-blue /
bordered secondary). Edit is an icon+label ghost (pencil + "Edit").
- Members count and the favorite star use btn-xs, closer to core's
channel-header favorite toggle; Members reads neutral.
- Generalize the port-to-core neutral treatment to also cover labeled
quaternary ghost buttons via a `docs-btn-neutral` marker, alongside the
existing `.btn-icon` rule (renamed button_icon_neutral -> button_neutral).
Rename components/form-controls -> components/form_controls (underscore,
matching the rest of the tree), updating all imports.
* fix(docs): grey hover for neutral toolbar buttons
btn-quaternary hovers with an accent-blue wash; the neutral icon/label
ghost buttons now use the grey channel-header hover instead.
* feat(docs): space menu, Settings modal, and Info panel
Space management from the title bar, backed by the real update/archive APIs.
- data/store: updateSpace (PATCH /spaces/{id}) and deleteSpace (DELETE)
data-source methods + thunks; UpdateSpacePatch type
- Space Settings modal (space_settings_modal): tabbed (Info / Permissions /
Configuration / Archive). Info edits name+description via updateSpace;
Archive confirms then deleteSpace + goHome. Permissions/Configuration are
PR #10 scaffolding (view_access, roles, external sharing).
- Space Info panel (space_info): read-only RHS mirroring core's Channel Info
— icon/title, description, members (avatars), page count + created time;
renders as the right column of the space view.
- title bar: the space name opens a dropdown menu (Space info, Members, Copy
link, Space settings, Leave, Archive; Settings/Archive gated on
canManageMembers). The details button is now the Info-panel toggle.
Built via subagents from an on-disk plan; reviewed and gates re-run here.
* fix(docs): match core btn-icon interaction states
Neutral toolbar buttons now mirror core's .btn-icon / channel-header icon:
grey base, light-grey hover, light-blue press (:active, declared after
:hover so a hovered press reads blue), and persistent blue when toggled on
(.active). The Info button gets `active` while its panel is open (alongside
aria-pressed) so the toggled state reads deep blue.
* fix:(docs) space header icon sizes
* refactor(docs): rename title/page bars to space/page header
Rename SpaceTitleBar -> SpaceHeader and PageBar -> PageHeader (files,
components, scss modules, imports, and stale comments).
* feat(docs): reorder space view; sliding pages sidebar
Stack space header (56px) and page header (52px, no bottom border) full
width, then a body row of the pages sidebar over the page content column.
The sidebar is always mounted and animates its width open/closed so the
tree slides in/out without reflowing.
* chore(webapp): bump preset-env browser floors
Raise preset-env targets to Chrome 110 / Firefox 102 / Edge 110 /
Safari 16.4 to match the host channels webapp's documented minimum
supported browsers.
* perf(webapp): code-split docs root; emit assets as files
Lazy-load the Docs backstage UI into its own async chunk (docs_root_lazy
+ React.lazy/Suspense) so the initial plugin bundle stays small. Switch
webpack output to publicPath: 'auto' with chunkFilename, and emit binary
assets via asset/resource instead of inlining, so chunks and assets
resolve from the served plugin path (works under subpath hosting). Add a
build:analyze target gated on webpack-bundle-analyzer.
* refactor(docs): share a 56px product header
Extract a reusable Header (56px bar, bottom border, left/right slots) and
adopt it in Docs Home and the space header, unifying the top-bar chrome.
Docs Home's header is now 56px tall to match.
* feat(docs): build out space view, favorites, modal infra
Large batch of Spaces UI work against the Figma specs, plus the shared
infrastructure it needed.
Shared UI:
- Menu refactored to children-based compound components (Item, LinkItem,
Separator, Submenu) and all callsites migrated
- Toast and modal controllers on base-ui, with imperative + hook APIs
- Tabs (vertical/horizontal), Select, Spacer, SaveChangesBar,
BasicInputModal, ResizableDivider; GenericModal gains titleActions and
optional header/footer dividers
- Button drops the required `type`, gains badging and icon/label modes
- WithTooltip is now the only tooltip; local tooltip component removed
Space view:
- Space header 56px, page header 52px, page tree, page content and hero
- Real resizable pages sidebar and full-height Space Info panel, with
editable description, menu and members sub-panel
- Page menu (tree + header), rename and delete modals, page drag preview
- Overview row for space home; truthful canDrop with blocked drop cues
Favorites:
- Spaces and pages favorited via the user preferences API, with memoized
selectors, tri-state space toggle and per-team sidebar ordering
- Ordering payload capped under the preference value limit
Data layer:
- Plugin reducers wrapped in an `entities` subtree
- Default-page routing via canonical Switch/Route/Redirect, with an
explicit /overview sub-route that outranks the default page
Context: iterative UI wiring session on docs-wire-spaces-api, driven by
Figma reviews and screenshots; verified with tsc, eslint, jest and build.
* feat(docs): make the page tree keyboard-navigable
Restructure the tree so `role="treeitem"` sits on the node wrapper that owns
the child group, giving the tree a single tab stop (roving tabindex) with
arrow-key traversal, Alt+arrow reordering, and Shift+F10/Menu to open a row's
actions. Reorder intent resolution lives in a pure `keyboard.ts` module so the
depth and sibling rules are testable without the DOM.
Add a `Readout` live region for the announcements this needs (moves, blocked
moves, create/move failures), and controlled open state on Menu/PageMenu so a
row can open its own menu from the keyboard.
Also folded in while touching these paths:
- gate the Home welcome hero on spaces actually being loaded, so it no longer
flashes for returning users
- extract `useLeaveSpace`, and surface a toast when archiving a space fails
- fix sidebar order to use prefixed favorite keys for both lists
- pass the resolved page into PageContent instead of re-selecting it
- tests for the tree panel, keyboard rules, leave-space, actions, and stores
Prompt: commit wip review changes
* feat(docs): let REST calls carry an abort signal
FetchOptions takes an optional AbortSignal, threaded through the rest helpers
and listAll so an in-flight paginated load can be cancelled instead of running
to the last page after the caller has moved on. Client4.getOptions already
passes signal through to fetch, so no host-side wiring is needed.
Aborts reject with fetch's AbortError rather than ClientError, keeping
"I cancelled this" distinguishable from a server rejection.
Prompt: FetchOptions should support an abort signal.
* feat(docs): adapt the webapp shell for the page editor
Pre-resolves the parts of the PR #13 (MM-69893) merge that are worth keeping
whichever order the two PRs land in, so the editor branch can be merged, reverted
and re-merged without redoing them.
- webapp_globals: adopt the document-mode editor contract — contentType,
extensions, onContentError, plus getEditor/hasContentError on the handle and a
hostSupportsDocumentEditor probe. A Docs page body is structured content, so
the editor is useless to us in markdown mode. Also onChange's param rename and
ReactNodeArray -> ReactNode[], which drops a deprecation warning.
- rest: thrown errors become RestError, a ClientError subclass that keeps the
parsed payload and the server error id. Draft publish answers a 409 with the
current page in the body, which ClientError alone discards. Existing
instanceof ClientError / status_code callers are unaffected.
- rest: add a path-relative doFetch over the same request path, for client
modules that describe a whole endpoint family.
- types: drop the stale note on Page — user_id and last_modified_by are here.
The editor branch's client layer compiles against this without edits, so it no
longer needs its own transport (which hand-rolled headers and relied on the
deprecated XMLHttpRequest CSRF fallback).
Prompt: merge PR#13 for preview with a revert-marked commit; conflict resolution
in a separate commit that stays, to reduce conflicts when both PRs merge
* feat(docs): address page edit mode with ?edit=1
Adds editPagePath plus isEditing/goToEditPage on useDocsNavigation. isEditing
requires a routed page, so the query is ignored on space and overview URLs where
there is nothing to edit.
Prompt: add an edit mode distinct from view mode, then make the page title
inline-editable
* feat(docs): add a PageTitle that reads or edits
A heading while reading, a field while editing, never both: view mode renders no
focusable control. The field composes the heading's type so switching modes moves
the text by nothing.
Prompt: add an edit mode distinct from view mode, then make the page title
inline-editable
* feat(docs): toggle page edit mode from the header
The header's Edit button was scaffolding with no handler; it now navigates to
?edit=1 and reads Done while editing. Layout is identical in both modes, so
nothing reflows on the toggle.
Prompt: add an edit mode distinct from view mode, then make the page title
inline-editable
* feat(docs): save an inline title edit on commit
Enter or blur writes through the existing updatePage action, which carries the
edit_at baseline; Escape reverts. A failed write keeps the typed title and toasts
rather than reverting, which would read as a silent loss.
Interim: this writes to the published page. When the draft DAL lands, commit
becomes a draft patch and the title stops being live until publish.
Prompt: add an edit mode distinct from view mode, then make the page title
inline-editable
* fix(docs): guard against duplicate title writes in flight
Enter and blur are independent commit triggers, so both can fire before a
prior write resolves and the store's page.title catches up — the unchanged
check alone let a second, redundant updatePage dispatch through on the same
stale baseline, which could surface as a spurious failure toast if the first
write's edit_at bump turned the second into a conflict. A ref-backed in-flight
flag now blocks the second dispatch while one is pending.
Prompt: fix review finding - duplicate-write race on rapid Enter-then-blur in
page_content.tsx PageTitleArea.commit
* fix(docs): commit the page title when edit mode exits
Every commit trigger was an event on the input, so exiting edit mode by URL
(Back, another page) unmounted the field without a blur and dropped what was
typed — while the heading went on rendering the buffer as if the write had
landed. Leaving edit mode is now a trigger in its own right, via the cleanup of
an effect keyed on `editing`, and reading mode renders the stored title rather
than the buffer.
The in-flight guard also swallowed a later, *different* edit, which the resync
effect then erased. A write now retries with whatever the buffer holds when it
settles, and the resync only adopts an incoming title when the buffer has no
unsaved input. The buffer is keyed on the page, so a newly routed page can never
inherit it and an in-flight write keeps targeting the page it started on.
Findings F1, F2 and the failed-commit and resync-coverage minors from the final
whole-branch review.
* fix(docs): make edit mode reachable by keyboard and SR
Activating Edit left focus on the header button, and the field that had just
appeared sat behind the whole page tree in tab order. The field now takes focus
when it mounts, which is exactly when edit mode opens. The toggle carries
`aria-pressed`, since a label changing from Edit to Done on a button that
already holds focus is not reliably re-announced.
Also records why `onValueChange` keeps its wrapper: Base UI passes event details
as a second argument, which `onChange` does not accept.
Finding F3 from the final whole-branch review.
* test(docs): cover the Edit and Done directions
Nothing exercised the branch that chooses between entering and leaving edit
mode: inverting it left the whole suite green. Hoisted it out of SpaceView as
`useTogglePageEditMode` — cheaper to test than a view that pulls in half a dozen
hooks — and covered both directions plus the no-page case.
Finding F4 from the final whole-branch review.
* feat(docs): make the spaces sidebar resizable
Applies the existing resize infra to the product sidebar, which was pinned at
264px: useSidebarWidth('spaces') for the persisted per-user width and the shared
ResizableDivider for the drag, so it gets the same snap-to-default, double-click
reset, and arrow-key resizing the pages sidebar already has.
The space view's Sidebar component is deliberately not reused — its open/close
slide and the width transition that comes with it don't apply here. Only the two
genuinely reusable pieces are, so no `resizing` class is needed: this box has no
width transition to suppress mid-drag.
Bounds are its own (220-420 rather than the pages sidebar's 200-480): it holds
team and space names rather than a page tree, so it needs less room to be useful
and less headroom before it crowds the content column.
Prompt: apply sidebar resizing (from the already-reusable infra) to the spaces
sidebar
* fix(docs): drop onto the space between page tree rows
The drop target was registered on the row itself, while row spacing came from
`gap` on three containers (.tree, .node, .children). Those 8px bands belonged to
no drop target, so a drag crossing them fired no onDrag and a release there found
dropTargets[0] undefined and did nothing — which is why only the rows' own edges
appeared to work.
Spacing is now padding on a new hit wrapper around each row, so adjacent
hitboxes touch and every pixel of the tree belongs to exactly one row. Each row
carries half the spacing above and below, keeping every adjacent pair 8px apart;
the tree's negative block margin cancels the outermost rows' share so the panel's
outer spacing is unchanged.
The hitbox now includes the spacing, which is also what the reorder indicator
already assumed: it was passed gap='8px' to draw itself in the middle of a gap
the hitbox didn't cover. That offset is no longer needed — the wrapper's edge is
the middle of the visual gap.
Prompt: bug in page tree: while dragging an item over the gap (between items), it
doesn't register
* fix(docs): make the last position at a level droppable
An expanded row's bottom edge claimed "after this item" while being drawn above
that item's own children, so the indicator pointed at one position and the drop
landed in another. For the last root item it was worse: that misleading band was
the only route to the last root position at all.
An expanded row no longer offers a bottom edge — its lower band becomes reparent
surface, so dragging low over it nests instead. The positions it used to conflate
are now separate and each reads the way it behaves: "first child" is the first
child's own top edge, which sits directly below the parent row; "last in this
group" is a new trailing strip after the group's last row.
The strip is rendered only where the ambiguity exists — when a group's last row
is expanded. A leaf or collapsed last row has nothing beneath it, so its bottom
edge is already truthful and sufficient. It also costs no layout: negative block
margins pull it over the padding its neighbours already reserve for spacing.
Prompt: the last item in the tree has children -- I don't have a natural way to
drop the item into the last (root) position
* fix(docs): quiet tooltips and indent the drop indicator
Two things that made a drag harder to aim than it needed to be.
Title tooltips are suppressed for the whole tree while a drag is in flight, not
just on the dragged row: the pointer is crossing rows to reach a drop position,
and a tooltip opening over the drop indicator covers the thing being aimed with.
The reorder indicator now starts at the destination's indentation instead of
spanning the panel. It's drawn inside the row's hit wrapper, which is full width
by design, but the page lands as a sibling of that row — so the line begins where
those rows begin. DropIndicator takes an `indent` for exactly this.
The group append strip gets the same treatment, moved from an indented box to a
full-width box with an indented line: the target stays easy to hit at any depth
while the line still shows the level the page would land at.
Prompt: when dragging a tree item, its tooltip should not appear; the drop
indicator is always full width, it should instead match the destination/reparent
indentation level
* fix(docs): layer stacked modals instead of overlapping them
The stack assumed Base UI handled nesting. It doesn't here: a Dialog.Root is
nested only when it renders inside another one's React subtree
(DialogRoot: `nested = Boolean(useDialogRootContext(true))`), and the controller
renders the stack as siblings — so Base UI saw unrelated dialogs and never
applied `data-nested-dialog-open` or `--nested-dialogs`.
With every modal sharing one `--z-index-modal`, paint order fell to whichever
portal mounted first, so a modal opened from another could land beneath it and its
backdrop was neither visible nor clickable. Depth is now explicit: the controller
knows the stack, so it supplies each modal's level and how many sit on top of it,
and each level paints in a two-slot band — its backdrop, then its popup.
Also gives the stack the layered feel it was missing. A covered modal scales back
slightly and fades, so opening one from another reads as a stack rather than a
replacement, and stops taking clicks so a lower popup is never under the pointer.
Only the topmost backdrop dims: two 50% blacks compound toward black, so lower
ones stay mounted for dismissal but paint nothing.
Corrects the store's doc comment, which asserted the behaviour that wasn't there.
Prompt: the modals layering is not right -- Space Settings then Archive space --
the archive modal doesn't appear to have a clickable or visible backdrop, and
Space Settings doesn't appear to go down a level
* fix(docs): layer a modal opened from inside another
Not an off-by-one: the previous commit's level and covered values were right, but
they only describe the modal stack, and these confirms never enter it. Space
Settings renders them in its own JSX — deliberately, per the comment there — so
they stack through Base UI's nesting instead, and nothing was reading that.
Two consequences, both fixed here:
Base UI renders no backdrop at all for a nested dialog (DialogBackdrop:
`enabled: forceRender || !nested`), which is why the archive confirm had nothing
visible to dim with or click away on. forceRender turns it back on, at a lower
alpha since the modal below is already dimming the app — full strength would
compound toward black with every level.
Base UI marks the covering popup with `data-nested-dialog-open` rather than
telling the child anything, so the recede now keys off that as well as the stack's
own signal. Both routes to stacking get the same treatment.
Depth is the sum of both: a nesting count from React context alongside the stack
level, so either route lands the dialog in the right paint band.
Base UI already hid the covered dialog from the accessibility tree — its title has
no queryable role while covered — so only the visual half was ever missing.
Prompt: is there an off-by-1 issue going on? still not seeing space settings go
down a level (or archive space go up a level)
* style(docs): keep covered modals opaque and animate modals in
A modal behind another is a solid sheet that moved back, not a translucent one, so
the covered state no longer fades the surface. The dimming was always available
from the covering modal's backdrop, which paints above the covered popup's band —
fading the popup as well doubled it and let the app show through a modal. Only
backdrops carry transparency now, and the nested one goes to 0.32 since it is the
whole layering cue rather than half of it.
Modals also transition in, through the attributes Base UI puts on the popup and
backdrop while they transition. Same duration, easing and scale as the receding
step, rising from the opposite direction, so opening a modal and sinking one behind
another read as one motion instead of two effects. The fade there is about a
surface that hasn't arrived yet; once it has, it stays opaque however deep it goes.
`.backdropCovered` is declared after `.backdropNested` on purpose: a backdrop can
be both, they have equal specificity, and transparent has to win.
Known gap: only the entrance animates. Closing removes the modal from the stack,
which unmounts it immediately, so Base UI never gets to run the exit — the
data-ending-style rules are styled but effectively unused until the stack keeps
entries mounted until their transition completes.
Prompt: layered modals should not become transparent, only the backdrops should
add visual layering with transparency; all modals should have a transition-in
effect to match the modal-layering transition
* fix(docs): animate modals out as well as in
Closing used to unmount the modal on the spot, so Base UI never got to run the
exit. The modal now owns its open state: a dismissal flips it, Base UI animates,
and the close is reported through onOpenChangeComplete — so whatever unmounts the
modal does it after the animation rather than cutting it off.
That covers Escape, the backdrop and the close button on its own. Buttons that
dismiss *and* act needed a way in, so a modal exposes useModalClose, which plays
the exit and then runs the handler; ConfirmModal's confirm and cancel go through
it. Its footer became a component to reach that context, since a footer passed as
a prop renders inside the modal but is built outside it.
Two things the tests caught:
`close?.(action) ?? action()` ran the action twice — closeWith returns void, so the
?? branch always fired as well. On a destructive confirm that meant archiving
twice. Written out as a conditional now.
Base UI waits for the popup's animations before reporting a close complete, and
jsdom runs none, so the wait never resolved and closes never completed in tests.
Its own escape hatch (BASE_UI_ANIMATIONS_DISABLED, see utils/useAnimationsFinished)
makes the callback immediate; set in the jest setup.
The two ConfirmModal handler tests now await the close, which is the behaviour
change rather than a weakened assertion: the handler runs once the modal has gone,
not on the click.
Prompt: please make the minimally necessary changes to also have the exit animate
as well.
* fix(docs): let modals animate in
The entrance styles were in place but never applied, for every modal. Base UI's
useTransitionStatus initialises `mounted` from `open`:
const [mounted, setMounted] = useState(open);
if (open && !mounted) { setMounted(true); setTransitionStatus('starting'); }
A dialog already open on its first render fails that condition, so the starting
state is skipped and `data-starting-style` never appears. Every modal here is
created already-open — openDocsModal mounts one, or a parent renders one — so none
of them could animate in.
The modal now mounts closed and opens in an effect, after the first paint, so Base
UI sees the false -> true change it needs. A ref guards the closed state it mounts
in, so a completion reported before it opens isn't mistaken for a dismissal and
doesn't close it on arrival.
Not covered by a test: the starting attribute is applied and cleared within a
frame, and jsdom runs no animations, so an assertion would pass whether or not the
entrance works. What is covered is that the modal still ends up open despite
mounting closed — every existing modal test asserts its content — and that closing
still completes.
Prompt: we still don't have any entrance animation (all modals, apparently)
* style(docs): soften how far a covered modal recedes
scale(0.96) translateY(-8px) -> scale(0.99) translateY(-2px), a quarter of the
previous travel. It was reading as the modal shrinking rather than stepping back.
The dim from the covering backdrop carries the layering; this only has to be enough
to notice.
The entrance keeps its larger travel: it has to bring a surface in from nothing,
where receding only hints at depth. Duration and easing stay shared, so the two
still read as one motion.
Prompt: reduce covered modal level transition effect by ~75% -- shouldn't scale
down as much, should be just enough to notice
* style(docs): sink a covered modal instead of lifting it
translateY(-2px) -> translateY(6px). It now settles downward as something opens over
it, so the covering modal reads as sitting in front rather than the two sliding
apart. Scale is unchanged.
Prompt: translate down 6px
* style(docs): settle a covered modal by 2px
Prompt: changed to 2px -- commit
* fix(docs): pin popups to their trigger inside a modal
Base UI positions a popup with `positionMethod: 'absolute'` by default, placing it
in document coordinates. The trigger can sit inside a modal, which is
`position: fixed` — a different coordinate space — so the popup and its trigger
drift apart as soon as anything scrolls. That is the landing-page dropdown in space
settings sliding away from its opener.
`positionMethod='fixed'` puts the popup in the same coordinate space the trigger is
actually in. It is equally correct outside a modal, so the select doesn't need a
prop for it.
Applied to the people picker too: it only ever opens inside the share modal, so it
had the same defect waiting.
Ruled out along the way: no transformed ancestor traps the fixed positioning (the
settings panes only set overflow), scroll tracking is active (autoUpdate runs since
neither popup sets keepMounted), and layering is fine — the host puts
--z-index-menu at 1100 above --z-index-modal at 1050.
Prompt: the default landing page dropdown in space settings is not correctly pinned
to the opener -- when you scroll the whole container moves up/down
* fix(docs): anchor the people picker popup to its field
Base UI anchors a combobox popup to its input:
const resolvedAnchor = anchor ?? (inputInsidePopup ? triggerElement : inputElement)
Here the input is a flex item inside the chips container — it sits after the chips
and shrinks as they are added — so the popup was measured and placed against that
remnant rather than the field. `--anchor-width` was the leftover input width, and
every chip re-anchored and resized the popup.
The positioner now takes the chips container as its anchor, which is the element the
popup should line up with. Both this and the positionMethod from the previous commit
are public props that pass straight through to floating-ui (strategy and position
reference) — no measurement or offset code of our own.
Prompt: the combobox open is still VERY broken
* fix(docs): stop the select popup detaching from its trigger
The real cause, and why the previous positionMethod change had no effect: Base UI's
alignItemWithTrigger defaults to true. In that mode the popup overlaps the trigger so
the selected item's text lines up with the trigger's value — macOS-style — and to
achieve it SelectPositioner gives the positioner a static style, passes
disableAnchorTracking, reports side 'none' and locks scrolling (lines 96, 110,
114-115).
So floating-ui's computed position was never used, the side/align/sideOffset we pass
were ignored, and nothing tracked the trigger. Inside a scrollable modal pane the
popup stayed where it first landed while the field scrolled away from it.
Turning it off makes this an ordinary anchored popup: tracking on, our placement
honoured, and positionMethod='fixed' now actually applied — which is what keeps it
aligned with a trigger inside a fixed modal.
Still no hacks: alignItemWithTrigger, positionMethod and anchor are all public props
that resolve to floating-ui's own strategy and position-reference.
Prompt: Space Settings > Default landing page STILL HAS THIS BUG
* fix(docs): keep resize handles clear of sidebar scrollbars
The handle is a 12px grab area straddling the panel edge, so its inner half sits
directly over the scrollbar of a panel that scrolls — and the handle wins the
pointer, making the scrollbar hard to grab. Both sidebars scroll, so both were
affected.
ResizableDivider takes an optional scrollbarClearance that slides it outward. At 6px
the hitbox clears the panel entirely, so the scrollbar is fully grabbable whatever
width the platform draws it — no assumption about scrollbar size.
The line moves with the hitbox rather than staying on the border: an indicator you
can see but not grab is worse than one a few pixels off. `max()` pins it to the
handle's inner edge once shifted, so it still reads as hugging the border rather than
floating in the content.
Opt-in, defaulting to 0, so a handle on a panel that doesn't scroll keeps straddling
the edge as before. Passed at the two call sites whose panels have overflow-y: auto.
Prompt: when a sidebar resize handle is near a scrollbar, it becomes hard to grab the
scrollbar -- adjust padding/margin either on resize handle, or at the callsite when
scrollbar is anticipated
* fix(docs): stop the pages sidebar clipping its resize handle
The slide-in/out transition clipped with `overflow: hidden` on the same
element the handle is positioned against, so the scrollbar clearance added
in 5f1adfe pushed the handle fully outside the clip and left almost nothing
to grab. Clipping now happens on a dedicated inner element, leaving the
handle free to hang over the edge.
Prompt: sidebar resize handle for the pages sidebar has a net width of
~1px, making it hard to grab
* fix(docs): keep the viewer in the space after deleting a page
Deleting the page being viewed sent the viewer to Docs home, ejecting them
from the space over a single page. It now lands on the space's home URL,
which keeps them in context and lets the default-landing-page redirect
apply, falling back to the space front door.
Also covers deleting an ancestor of the routed page: subpages go with their
parent, so the exact-id check left the viewer on a page that no longer
existed, relying on SpaceView's not-found redirect to bounce them to
/overview. Containment is now a selector over the same subtree walk the
delete action prunes with.
The arrival replaces rather than pushes, since the URL being left names a
page that no longer exists.
Prompt: fix a routing issue when deleting a page -- it currently reverts to
Home (instead, it SHOULD stay in the space, either back to space overview,
or the configured default page)
* fix(docs): place a reordered page where it was dropped
The optimistic reindex numbered a sibling group 0-based while the server
numbers it 1-based, and a move's response upserts only the moved page. That
page therefore landed among its siblings one number high, tying with the one
below it, and the title tiebreak resolved the tie against it — so a page
dropped between two siblings settled one position too low.
Numbering now matches the server, and the moved page's old group is left
alone rather than closed up, since store.MovePage reindexes the destination
group only.
Prompt: when dropping an item between two root pages, it will be placed +1
down from where it should be
* feat(docs): open a newly created page in edit mode
A new page is empty and titled "Untitled", so reading it is never what was
wanted — creating one now routes to ?edit=1, which also focuses the title
input, the first thing to fill in.
Prompt: when a page is created, let's default enter into edit mode
* fix(docs): replace rather than push when creating a page
Opening the new page's edit URL took over the entry for wherever the add
button was clicked, instead of stacking on top of it.
Prompt: creation should replace, not push
* Revert "fix(docs): replace rather than push when creating a page"
This reverts commit 484cd5898b5371f41f84d796f5c273c0e5d3fe4c.
* feat(docs): edit an unnamed page's title as a placeholder
The server rejects an empty title, so a created page stores the literal
"Untitled" — which meant naming a new page started by selecting and clearing
that word. The title field now shows an unnamed page as empty with
"Untitled" as its placeholder, and maps back on write: clearing the field
returns the page to unnamed rather than failing validation.
The sentinel moves to one shared descriptor, so the string that gets stored
and the string recognized as "unnamed" cannot drift apart. It needs
defineMessage to stay extractable.
Also corrects a test that claimed the server accepts an empty title.
Prompt: Do pages support empty string titles? Having more of a placeholder
UX in edit mode would be better (so users don't need to highlight 'Untitled'
to clear)
* feat(docs): add the drafts data and store layer
Drafts as a first-class slice, ported from PR #13's client onto this branch's
data-source seam so there is one transport rather than two.
- types/drafts.ts: Draft, DraftSummary, DraftPatch, publish conflict shapes
- DocsDataSource gains create/get/update/delete/list/publish for drafts; a
missing draft normalizes to undefined rather than a 404 the caller catches
- PublishConflictError unpacks publish's 409, which uniquely carries data (the
current server page), and distinguishes forceable conflicts from an
unpublished parent, which force cannot fix
- drafts + draftsInSpace reducers, keyed by page id and kept separate from
pages so one user's unpublished title cannot render in another's tree
- PUBLISHED_DRAFT is one action, removing the draft and adding the page
together, so no render shows the new page twice
- selectors distinguish unpublished edits to a page from an orphan draft (an
unpublished page), which is the duplicate-row trap encoded once
Thunks cover fetch/create/save/discard/publish. No UI consumes these yet.
Prompt: Back to Drafts DAL/Reducers/Selectors & interop/integration with Page
Tree and Pages DAL/hooks
* feat(docs): create pages as drafts and publish them
Adding a page now creates a draft rather than a published page, so nobody
else sees a half-written page and the author decides when it becomes real.
The draft reserves its page id, which is what makes it addressable before it
exists.
That gives /spaces/:spaceId/drafts/:pageId a job and settles the view-vs-edit
question the interop plan left open: ?edit=1 is a mode on a published page,
the draft route is an unpublished one. A draft has no reading mode, so its
route is always editing and carries no query.
- useCreateRootPage creates a draft and opens it
- SpaceView renders the draft route, redirecting to the page once the draft is
published (here or in another tab) and to the front door when the URL names
no draft — but only once the fetch settles, so a slow response is not
mistaken for a bad URL
- PageContent takes a page or a draft and routes title writes to the matching
endpoint; the placeholder, in-flight guard and exit-commit behaviour are
unchanged
- Publish in the page header, with the unpublishable-parent conflict given the
instruction that helps rather than a generic retry
Prompt: creation + draft route + publish first, then the tree refactor
* feat(docs): show unpublished pages in the page tree
Orphan drafts — pages that exist only as the author's draft — now render as
tree rows marked "Draft", visible only to their author.
PageNode carries `id`/`title` directly with `page`/`draft` saying what backs
the row, so the five consumers that keyed off `node.page.id` work for either
kind without branching.
Placement follows from the server's model rather than preference: a draft has
no SortOrder column, so it cannot be interleaved and sorts last in its group —
which is also where publishing appends it, so the position shown is the
position it will take. Drafts order among themselves by create_at, not
update_at, which advances while the author types.
The published/draft boundary is enforced where a move is resolved:
- a draft is neither a drag source nor a drop target, so the positions around
it are never offered
- Alt+arrow refuses to reorder a draft, and stops a page at the last published
sibling instead of letting it cross
- each group's append strip renders between its published rows and its drafts,
since that is where an appended page actually lands
A draft is never a parent — its children could not be published until it was,
and a published page cannot be parented to one at all — so a draft parented to
another draft falls back to the root group rather than nesting or vanishing.
Prompt: keep drafts at the end of the page tree, and block any reordering
operations that would drop a published page AFTER drafts
* feat(docs): make page tree rows links and give drafts a menu
Rows were divs with an onClick, so a page could not be opened in a new tab,
middle-clicked or copied — and a draft row was not clickable at all, because
nothing routed it to its draft address.
Each row is now an anchor to the address that fits what backs it: the page
path for a published page, the draft path for an unpublished one. Enter and
Space go through the same helper, so keyboard and pointer cannot disagree. The
link's ::after stretches over the row, which keeps the whole row clickable
while leaving the chevron and the menu as real buttons — they could not be
nested inside an anchor.
Draft rows keep the overflow menu with the subset that means anything for an
unpublished page: copy link, open in new window, and Discard. Discarding
destroys the only copy of the page, so it confirms first and, if you are
looking at it, returns you to the space.
Prompt: draft pages should still have the dotmenu in the page tree (with a
reduced options set); add a Discard entry; all items in the page tree need to
be proper <Link/>s
* feat(docs): give drafts view/edit modes and a publish control
A draft now reads the same ?edit=1 as a published page: an unpublished page
still has a rendered form worth reading before deciding to publish it. The
query rides on whichever address is current, so toggling never moves between
the page and draft routes.
Header controls, in edit mode: Publish or Update first, Close beside it. Done
is renamed Close, since it leaves the mode rather than completing anything —
title edits are already saved by then.
Publish and Update are the same act on either side of publication: with no
published page the draft is the page, and with one it is unpublished edits to
it. Update is therefore disabled until there are edits to apply, saying so
rather than failing when pressed. It only becomes reachable once the autosave
editor lands; the plumbing is in place for it.
The routed page's draft is now loaded for every page, not just the draft
route, since a published page can have one too.
Prompt: draft pages should support the same edit/view modes as pages, with the
added option of publishing. Done should instead be Close. Drafts should also
have Publish. Published pages should have an Update button. Close on the
right, Publish/Update on the left
* feat(docs): make spaces sidebar rows links
Space rows and Home were buttons calling history.push, so neither could be
opened in a new tab, copied, or middle-clicked. SidebarItem now renders an
anchor when given a destination and stays a button for rows that act rather
than navigate.
Space rows opt out of the browser's own link drag, which would otherwise
pre-empt drag-to-reorder.
The nav's active state is derived from the routed space id rather than held in
component state, so a back/forward navigation can't leave it out of sync — it
was tracked separately only because the click handler set it.
Prompt: Similarly to page tree items (that they need to be <Link/>s) -- Space
sidebar items also should be proper <Link/>s
* fix(docs): unclip row titles and drop link underlines
The page node's <Spacer/> was a second flex:1 sibling next to .rowLink,
splitting the free space with it — so the title's box stopped halfway
across the row and the Draft badge and favorite star floated in the
middle. .rowLink already grows; the spacer was redundant.
The stretched hit area (.rowLink::after) also painted over the title,
so hit-testing resolved the pointer to the anchor and the title never
saw the mouseenter its truncation tooltip listens for. Lifted it above,
as the chevron and menu already were.
Spaces sidebar rows never set text-decoration, so the host's anchor
styles underlined them once they became links. They now get the same
overflow tooltip as page tree rows, replacing a native title attribute
that fired whether or not the label was clipped. The space dot menu's
tooltip drops the space name, which the row it sits on already gives.
Context: reported clipped/underlined rows and missing node tooltips
while reviewing the Spaces UI branch.
* feat(docs): offer Publish while reading a draft
An unpublished page is invisible to everyone but its author, so the
action that fixes that shouldn't require a detour through edit mode.
Update stays editing-only: it applies edits already made, which is
nothing a reader of a published page needs offered.
Context: "Publish should still be available to drafts in read mode".
* feat(docs): add a reusable RHS with routed open state
Space info was the only right-hand panel and owned its whole frame, so
a second one had nothing to build on. RhsPanel now holds the frame —
resizable column, product-header chrome, heading, close, optional back
— and panels supply only their contents. Its `name` doubles as the
region's accessible name and the interpolation in the resize and back
labels, replacing three space-specific strings with generic ones.
Open state moves from local state into the URL via useRhs, so it
survives a reload and can be linked. Every change replaces the history
entry: a panel is a view of the page, not a place, and pushing would
make Back mean "hide the panel" for as many times as the reader toggled
one. An unrecognized ?rhs= reads as closed.
The Comments control in the page header now opens a placeholder panel —
the frame and routing are real, the contents are not.
withQuery() edits the existing query in place, so edit mode and the open
panel no longer clobber each other: toggling the editor kept only
?edit=1 and would have closed the panel.
Context: "Is it in a reusable pattern? If not, let's add a
dummy/placeholder RHS for the Comments button... would also be nice to
have routing integration built in. Opening or closing an RHS shouldn't
push a new location to the history stack, but rather replace it."
* feat(docs): add a fullscreen mode for the page
Expand now hides the spaces sidebar and the space header grows an
"All spaces" control in its place, per the design.
The two ends of this sit four components apart with nothing to share:
the sidebar belongs to the product root, the control to the page header.
So the mode travels in the URL like edit mode and the open panel, and
both ends read useFullscreen instead of a flag threaded through
DocsMainContent and SpaceView. Toggling replaces the history entry —
Back should leave the page, not undo a resize of the view.
"All spaces" reuses goHome, whose bare path drops the query, so leaving
fullscreen is part of going there. That one pushes: it goes somewhere.
Expand's label switches with the mode, so both descriptors are hoisted
to defineMessage — a bare object inside a conditional isn't statically
analyzable and extraction missed both.
Context: mini-spike from Figma node 4526-68777. Two open questions in
the branch notes: the control is in the space header (top row) rather
than the page header, and fullscreen resets on navigation between pages
since page links carry no query.
* feat(docs): resolve routed ids against the server
A space or page id in the URL was resolved only against whatever the
list fetches had put in the store: docs_main_content redirected to home
once the team space list had loaded without the id, and space_view
redirected to /overview once the space's page list had. The list's
silence is not an answer, though — it can predate the entity, or belong
to another team — so a deep link could bounce out of something that was
really there.
Add getPage to the data source (getSpace already existed but no thunk
called it) plus fetchSpace/fetchPage thunks, and resolve routed ids
through useRoutedSpace/useRoutedPage. Each asks the server for the id
only while the entity is absent, and reports `resolved` so the redirect
waits for a real answer instead of the list's silence. Fetching by id is
also the only source of a page body, since the list returns summaries.
The draft route passes fetchMissing: false — a draft reserves its page
id before publishing, so the page legitimately does not exist and asking
for it would 404 on every draft opened.
Context: users could not reach spaces others had created; the server
gate behind that is held back for PR #10 (MM-69269), so this covers only
the client half — ID-in-URL navigation asking the server for that id
rather than relying on list/initial-data fetches to populate the store.
* docs(docs): spec Share modal member add/remove wiring
Design for wiring the Share Space modal to the existing add/remove
member routes: data source method, store action types and reducer
splices, thunks, a useManageSpaceMembers hook owning fan-out and
toasts, an Add button, and a per-row menu offering Remove or Leave.
Records that the add-member route already exists on master — two
comments in share_space_modal.tsx claiming otherwise are what kept
this unwired — and scopes roles/view-access out to PR #10.
Context: brainstormed decisions were chips+Add over add-on-select,
remove under the existing role trigger, and self-removal delegating
to useLeaveSpace.
* docs(docs): add space members on select, drop the Add button
Adding one member is a whole action: it is the unit the server route
accepts and the unit the reducer splices, so the picker commits on
select rather than accumulating pending chips behind an Add button.
Bulk can be layered on later as a wrapper over the same primitive
without changing its signature.
Removes the pending state, the allSettled fan-out and the partial-batch
toast; reduces PeoplePicker to single-select, which drops the chip
parts. Every add failure can now name its user, since an add is always
one user. Records the trade: no review step before access is granted,
offset by Remove being one menu item away.
Context: follow-up to the approved spec — keep it simple for now.
* docs(docs): layer bulk member add over the singular thunk
Restores the chips + Add button flow, but keeps the batching out of the
singular action. addSpaceMember stays a primitive that dispatches once
and rejects; a separate addSpaceMembers thunk calls it per user through
Promise.allSettled and resolves with the users that failed and why.
Bulk is therefore a caller of the primitive, not a generalisation of it:
a single add still reads naturally anywhere, successes in a partly-failed
batch have already dispatched, and there is no bulk action type because a
partial batch has no single truth to express. Returning the raw error per
user keeps message selection in the hook.
PeoplePicker is back to untouched — its existing multi-select chip UI is
what this flow wants.
Context: follow-up — no batch hacks in the singular actions/thunks.
* docs(docs): share member internals across three surfaces
The member roster is written three times and none of the copies work:
the Share modal's picker is wired to nothing, the Permissions tab has
hand-rolled rows plus a search field that is an aria-disabled div, and
the space info panel is a third read-only copy with its own ids.
Adds a components/space_members/ core — roster, row, row menu, add field
— that each surface wraps with its own chrome. Read-only is expressed as
an absent actions bag rather than a readOnly flag, so a row can never
render a menu with nothing behind it. Consolidates the three duplicated
message id sets under docs.spaceMembers.*, extracts PermissionsTab out
of the 592-line settings modal, and sequences the core to be built with
its first consumer then proven by two ports.
Notes that membership changes stay out of the settings modal's
dirty/save flow: an add is already committed when it returns, so
SaveChangesBar would imply a discard that cannot happen.
Context: port Share dialog internals to Space Settings -> Permissions;
one core, different wrappers and UX flows.
* docs(docs): plan space member add/remove implementation
Nine TDD tasks bottom-up: reducer splices, data source plus the singular
and bulk thunks, the membership hook, then the shared core in three
increments (read-only roster, row menu, add field) followed by the three
wrappers.
Each task carries its own failing test, the code to make it pass, and a
commit. Signatures were read out of the source rather than guessed; the
one open question — how Menu.Item renders disabled — is called out with
where to check.
Deviates from the spec in one place: removing another member gets its own
last-member string, since useLeaveSpace's ends "before you leave".
* feat(docs): splice one space member in the store
* feat(docs): add space member thunks, bulk over singular
* feat(docs): add a space membership hook with its messages
Prompt: implement Task 3 of the space-members SDD plan — the
useManageSpaceMembers hook that sits above the Task 1-2 reducer/thunk
layer, translating add/remove/leave outcomes into user-facing toasts.
* feat(docs): add a shared space member roster
* feat(docs): add a member row menu with remove and leave
* feat(docs): add a shared add-members field
* feat(docs): wire Share modal to member add/remove
* fix(docs): order action_types import in tests
* feat(docs): rebuild permissions tab on member core
* feat(docs): read-only roster in space info panel
* fix(docs): close settings modal on leaving a space
Final whole-branch review found four gaps in the space-members work:
- Permissions tab left Space Settings open on a space the user just
left (Share modal already closed itself on leave); thread onClose
into PermissionsTab and mirror that behavior.
- add.error.single's non-403 branch had no test, so an implementation
that always emitted the not-on-team message would still pass.
- The busy Remove-item test asserted aria-disabled but never clicked
it, so a click handler that still fired would pass.
- The member menu trigger's accessible name ("Manage {name}") didn't
contain its visible text ("Admin"), failing WCAG 2.5.3.
Prompt: fix wave from final review of the space-members branch (see
.superpowers/sdd/2026-08-06-space-members/final-fix-report.md).
* fix(docs): indent a page ahead of its new parent's drafts
The indent branch counted every child of the new parent, drafts
included, so a page indented under a parent holding both published
pages and drafts claimed a position past the published tail — one the
server cannot store. The down branch already guards this with
publishedCount; indent now does the same.
Context: /longshot --only review — Phase 6 review of the Spaces UI
branch. Found by the page-tree review lane and confirmed against the
invariant keyboard.ts already documents.
* test(docs): cover how a drop resolves to a move
resolveMove and computeDropTarget decide where a dragged page lands,
and had no tests at all — while resolveReorder, their keyboard
counterpart, was fully covered. The indent bug fixed in the previous
commit was exactly this class of defect, caught only because the
keyboard path was tested.
Both are exported to be tested directly, as the keyboard resolver
already is.
Context: /longshot --only review — Phase 6 review of the Spaces UI
branch.
* feat(docs): confirm before removing a member or leaving
Removing someone and leaving a space both took effect on click, alone
among the destructive actions here — deleting a page, discarding a
draft and archiving a space all confirm first. A misclick revoked
someone's access, or ejected the clicking user, with no undo.
The confirmation lives in the shared roster rather than in each of the
three surfaces that render it, so the copy stays in one place and a new
surface cannot forget it.
Both handlers close the modal themselves: the dialog runs its confirm
handler instead of its own onClose, so one that doesn't close leaves an
entry on the stack. Covered by a test asserting the stack empties.
Context: /longshot --only review — Phase 6 review of the Spaces UI
branch.
* feat(docs): offer to publish over a page that changed
Publishing a draft whose page moved underneath it reported a generic
"please try again", and retrying reissued the same refused request —
there was no way out. The conflict already carried everything needed to
recover: the server's current page, and whether force can resolve it.
A conflict now names the other version and offers to publish anyway.
An unpublished parent still gets its own instruction, since force
cannot resolve that one.
The hook becomes .tsx to render the dialog, as components that open
modals already do.
Context: /longshot --only review — Phase 6 review of the Spaces UI
branch.
* fix(docs): dismiss only the top modal on Escape
Stacked modals are pushed onto the docs modal stack and rendered as
siblings, so Base UI sees no parent/child relationship between them and
every open dialog answered the same Escape — one press closed the whole
stack. Covered modals now ignore it.
Modals nested through JSX are a real Base UI parent/child pair, which it
guards itself, so they never reach this covered.
Latent until now: every other stack modal opens from a header, menu or
panel. The member confirmations are the first opened from inside another
stack modal.
Context: /longshot --only review — Phase 6 review of the Spaces UI
branch. Reproduced with two stacked modals before fixing.
* chore(docs): re-extract the i18n strings
The catalogue had drifted: the space-member strings were never extracted
after the roster moved into its own component, and four keys renamed by
that move were left behind. Adds the new confirmation and publish-
conflict strings in the same pass.
Context: /longshot --only review — Phase 6 review of the Spaces UI
branch.
* chore(docs): drop stale superpowers plan and spec
Prompt: remove docs/superpowers artifacts (old/noise)
* fix: harden spaces UI data handling
* fix(docs): harden space workflows and modal stacking
* fix(docs): scope neutral button styles
* fix(docs): seed drafts with an empty paragraph
* fix(docs): tighten routed and member hooks
* fix(docs): settle routed page lookup failures
* fix(docs): parse empty content as a paragraph
* fix(docs): address review findings
* fix(docs): address minor review feedback
* fix(docs): address follow-up review feedback
calebroseland
force-pushed
the
MM-69893-page-editor-autosave
branch
from
August 11, 2026 21:59
2abafeb to
0f1571b
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
webapp/src/components/space_view/page_content/page_content.tsx (1)
55-76: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftSave titles through the draft when a published page is being edited.
When
pageanddraftboth exist, Line 55 ignoresdraft.titleand Lines 71-72 callupdatePage. This publishes the title immediately while the body remains unpublished. A later Update can publish the draft's older title and revert the rename.Prefer the draft title when it exists. Save title changes with
saveDraftfor both unpublished pages and published pages under edit. Add a regression test for a published page with an existing draft title.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webapp/src/components/space_view/page_content/page_content.tsx` around lines 55 - 76, Update the subject selection to prefer draft data when both page and draft exist, and change saveTitle to call saveDraft whenever a draft is present, falling back to updatePage only when no draft exists. Add a regression test covering a published page with an existing draft title and verifying the title is saved through the draft path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@webapp/src/components/page_editor/page_editor.module.scss`:
- Around line 51-55: Add automatic inline margins to the constrained child
selector under .pinnedToolbar so its max-width content is centered on wide
viewports, while preserving the existing width, max-width, and padding rules.
In `@webapp/src/components/page_editor/toolbar_controls.tsx`:
- Around line 89-100: Add an onKeyDown handler to the trigger button in the
toolbar control, alongside its existing onClick handler, that detects Escape and
closes the menu via setOpen. Preserve the current toggle behavior and avoid
affecting other keys.
In `@webapp/src/hooks/draft_autosave.ts`:
- Around line 84-85: Update the body construction in the draft autosave flow to
determine baseline presence by checking against undefined rather than
truthiness, so a base_edit_at value of 0 is included while only an absent
baseline uses entry.patch unchanged; preserve the existing saveDraft dispatch
behavior.
In `@webapp/src/hooks/page_presence.ts`:
- Around line 74-75: Update the expiration comparison in the page-presence
snapshot logic to use >= so snapshots expire exactly at snapshot_at +
active_timeout_ms. Add a test covering time advanced precisely to
active_timeout_ms and verify it returns no active editors.
In `@webapp/src/hooks/pinned_toolbar.ts`:
- Around line 20-27: The toggle callback in
webapp/src/hooks/pinned_toolbar.ts:20-27 must account for repeated calls before
a re-render by tracking the intended pinned state immediately or blocking
subsequent toggles until the update completes; ensure consecutive calls save
false then true. Add a test in webapp/src/hooks/pinned_toolbar.test.tsx:51-62
that invokes the callback twice within one act block and verifies those saved
values.
---
Outside diff comments:
In `@webapp/src/components/space_view/page_content/page_content.tsx`:
- Around line 55-76: Update the subject selection to prefer draft data when both
page and draft exist, and change saveTitle to call saveDraft whenever a draft is
present, falling back to updatePage only when no draft exists. Add a regression
test covering a published page with an existing draft title and verifying the
title is saved through the draft path.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 148f0684-fa10-494d-94a4-d0865466076a
⛔ Files ignored due to path filters (1)
webapp/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (29)
webapp/i18n/en.jsonwebapp/package.jsonwebapp/src/client/presence.tswebapp/src/components/page_editor/floating_formatting_bar.module.scsswebapp/src/components/page_editor/floating_formatting_bar.tsxwebapp/src/components/page_editor/page_editor.module.scsswebapp/src/components/page_editor/page_editor.tsxwebapp/src/components/page_editor/toolbar_controls.module.scsswebapp/src/components/page_editor/toolbar_controls.tsxwebapp/src/components/page_editor/toolbar_slot.tsxwebapp/src/components/space_view/page_content/page_content.module.scsswebapp/src/components/space_view/page_content/page_content.tsxwebapp/src/components/space_view/page_header.tsxwebapp/src/components/space_view/space_view.tsxwebapp/src/hooks/autosave_status.tswebapp/src/hooks/draft_autosave.test.tsxwebapp/src/hooks/draft_autosave.tswebapp/src/hooks/drafts.tsxwebapp/src/hooks/editor_content.test.tsxwebapp/src/hooks/editor_content.tswebapp/src/hooks/host_editor.tswebapp/src/hooks/page_editing.tswebapp/src/hooks/page_presence.test.tsxwebapp/src/hooks/page_presence.tswebapp/src/hooks/pending_saves.test.tswebapp/src/hooks/pending_saves.tswebapp/src/hooks/pinned_toolbar.test.tsxwebapp/src/hooks/pinned_toolbar.tswebapp/src/index.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- webapp/src/index.tsx
- webapp/package.json
- webapp/src/hooks/draft_autosave.test.tsx
Co-authored-by: Cursor <cursoragent@cursor.com>
nang2049
force-pushed
the
MM-69893-page-editor-autosave
branch
from
August 12, 2026 14:29
0f1571b to
3b8b14d
Compare
calebroseland
approved these changes
Aug 12, 2026
Willyfrog
added a commit
that referenced
this pull request
Aug 14, 2026
…ed segments Master now carries #12's Spaces/Pages data layer and #13's editor, which is what the import UI was waiting for: fetchSpaces/fetchPages read the plugin REST API, so a Space the importer creates finally appears in the sidebar, and the refresh this branch already dispatched on completion stops being a no-op. Resolved: - client/rest.ts, rest.test.ts: master's, wholesale. This branch carried a copy of the same file so there would be one HTTP layer rather than two; master's has since grown a basename fallback in siteRoot, and apiUrl is still exported, so client/imports.ts needed nothing. - routing/paths.ts: applied the underscore rule the same way to every non-content segment, so master's `overview` joins `_drafts` and `_import` as `_overview`. Its safety was circumstantial — it sits exactly where a page id goes and would hide any page addressed that way the moment user-chosen slugs return, which the URL scheme still advertises. RESERVED_SEGMENTS now covers all three and paths.test.ts asserts each is unmatchable as an id. - hooks/navigation.ts: master's overview/edit/RHS state plus this branch's isImport and goToImport, with the import routes matched ahead of DOCS_ROUTE. - docs_root.tsx: re-applied the routed import panel into master's restructured root (modal controller, readout, toaster, resizable sidebar, fullscreen). - space_item_menu.tsx: master rewrote the menu from a MenuItemSpec array to declarative Menu.Items; the import entry is re-expressed in that form. - navigation.test.tsx, paths.test.ts: both suites kept, master's URLs moved onto the renamed segments. Its "not editing on the overview URL" case was failing for the right reason — a bare `overview` now parses as a page id — which is the collision the rename removes. 550 webapp tests across 76 suites, server suite green, golangci-lint clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Aug 20, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
POC for the Docs page editor. Mounts the core webapp's
WysiwygEditorat the page route in document mode and wires it to the draft API added in MM-69271.Built on the plugin surface published in MM-69912 (core). Uses all five additions from that contract:
contentType='json',extensions,onContentError,getEditor()andhasContentError().base_edit_atsent on every write for optimistic locking. Writes are serialized so publish cannot race an in-flight save, and a pending patch is flushed to the page you are leaving rather than dropped.concurrent_editfromconcurrent_autosave.page_presence_updatedwebsocket events, currently rendered as a count.FormattingBar, pinned to the top by default with a toggle to a floating bar over the selection.calloutnode already on the server allowlist.** Not in this PR **
Read/view mode, the in-page outline sidebar, presence avatars, and the text-colour / comment / AI toolbar controls.
are untouched.
Testing
Requires
MM_FEATUREFLAGS_ENABLEDOCS=true, otherwise every plugin API call is a 501.webapp/src/hooks/draft_autosave.test.tsxcovers debouncing, coalescing,base_edit_at, cancellation, in-flight flushing, and failure reporting.Manually: type and confirm the indicator settles on Saved, go offline and publish, confirming the editor stays open with an error rather than closing and losing the text; type and immediately switch pages, confirming the text is on the page you left.
Open questions for the team
localStorage, which is per-browser, should it be a real user preference?CommandProvideron any text starting with/, so typing/in a page currently autocompletes channel slash commands. Should/instead open a docs block-insert menu? This needs a decision before it can be fixed properly in core.Known issues that need fixing in Core
RangeErrorfromwysiwyg_editor.tsxand crashed the whole webapp. Guarded locally.isOpenfalse but the next keystroke re-runs the providers and reopens it.WysiwygSuggestionListhardcodesposition='top', worked around here with aMutationObserverthat re-anchors the popup to the caret.Ticket Link
https://mattermost.atlassian.net/browse/MM-69893